[PATCH v4 0/4] Implement vdpasim stop operation

[PATCH v4 0/4] Implement vdpasim stop operation

[Eugenio Pérez]

Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
that backend feature and userspace can effectively stop 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).

After the return of ioctl with stop != 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 stop == 0, the device can continue
processing buffers as long as typical conditions are met (vq is enabled,
DRIVER_OK status bit is enabled, etc).

In the future, we will provide features similar to VHOST_USER_GET_INFLIGHT_FD
so the device can save pending operations.

Comments are welcome.

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 stop operation
  vhost-vdpa: introduce STOP backend feature bit
  vhost-vdpa: uAPI to stop the device
  vdpa_sim: Implement stop 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                 | 34 +++++++++++++++++++++++++++-
 include/linux/vdpa.h                 |  6 +++++
 include/uapi/linux/vhost.h           | 14 ++++++++++++
 include/uapi/linux/vhost_types.h     |  2 ++
 8 files changed, 83 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH v4 1/4] vdpa: Add stop 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 15af802d41c4..ddfebc4e1e01 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -215,6 +215,11 @@ struct vdpa_map_file {
  * @reset:			Reset device
  *				@vdev: vdpa device
  *				Returns integer: success (0) or error (< 0)
+ * @stop:			Stop or resume the device (optional, but it must
+ *				be implemented if require device stop)
+ *				@vdev: vdpa device
+ *				@stop: stop (true), not stop (false)
+ *				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
@@ -316,6 +321,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 (*stop)(struct vdpa_device *vdev, bool stop);
 	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 v4 2/4] vhost-vdpa: introduce STOP backend feature bit

[Eugenio Pérez]

Userland knows if it can stop the device or not by checking this feature
bit.

It's only offered if the vdpa driver backend implements the stop()
operation callback, and try to set 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 1f1d1c425573..32713db5831d 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_stop(const struct vhost_vdpa *v)
+{
+	struct vdpa_device *vdpa = v->vdpa;
+	const struct vdpa_config_ops *ops = vdpa->config;
+
+	return ops->stop;
+}
+
 static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
 {
 	struct vdpa_device *vdpa = v->vdpa;
@@ -575,7 +583,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_STOP)))
+			return -EOPNOTSUPP;
+		if ((features & BIT_ULL(VHOST_BACKEND_F_STOP)) &&
+		     !vhost_vdpa_can_stop(v))
 			return -EOPNOTSUPP;
 		vhost_set_backend_features(&v->vdev, features);
 		return 0;
@@ -624,6 +636,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_stop(v))
+			features |= BIT_ULL(VHOST_BACKEND_F_STOP);
 		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..2758e665791b 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
+/* Stop device from processing virtqueue buffers */
+#define VHOST_BACKEND_F_STOP  0x4
 
 #endif
-- 
2.31.1

[PATCH v4 3/4] vhost-vdpa: uAPI to stop the device

[Eugenio Pérez]

The ioctl adds support for stop the device from userspace.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 drivers/vhost/vdpa.c       | 18 ++++++++++++++++++
 include/uapi/linux/vhost.h | 14 ++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 32713db5831d..d1d19555c4b7 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
 	return 0;
 }
 
+static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
+{
+	struct vdpa_device *vdpa = v->vdpa;
+	const struct vdpa_config_ops *ops = vdpa->config;
+	int stop;
+
+	if (!ops->stop)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&stop, argp, sizeof(stop)))
+		return -EFAULT;
+
+	return ops->stop(vdpa, stop);
+}
+
 static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
 				   void __user *argp)
 {
@@ -650,6 +665,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_STOP:
+		r = vhost_vdpa_stop(v, argp);
+		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..c7e47b29bf61 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)
 
+/* Stop or resume a device so it does not process virtqueue requests anymore
+ *
+ * After the return of ioctl with stop != 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 stop == 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_STOP			_IOW(VHOST_VIRTIO, 0x7D, int)
+
 #endif
-- 
2.31.1

[PATCH v4 4/4] vdpa_sim: Implement stop vdpa op

[Eugenio Pérez]

Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
that backend feature and userspace can effectively stop 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).

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 50d721072beb..0515cf314bed 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_stop(struct vdpa_device *vdpa, bool stop)
+{
+	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+	int i;
+
+	spin_lock(&vdpasim->lock);
+	vdpasim->running = !stop;
+	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,
+	.stop			= vdpasim_stop,
 	.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,
+	.stop			= vdpasim_stop,
 	.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 v4 0/4] Implement vdpasim stop operation

[Parav Pandit]

> From: Eugenio Pérez <eperezma@redhat.com>
> Sent: Thursday, May 26, 2022 8:44 AM

> Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> 
> that backend feature and userspace can effectively stop 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).
> 
> 
> 
> After the return of ioctl with stop != 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 stop == 0, the device can continue
> 
> processing buffers as long as typical conditions are met (vq is enabled,
> 
> DRIVER_OK status bit is enabled, etc).

Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.

Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?
This is in the context of other discussion we had in the LM series.

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit via Virtualization]

> From: Eugenio Pérez <eperezma@redhat.com>
> Sent: Thursday, May 26, 2022 8:44 AM

> Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> 
> that backend feature and userspace can effectively stop 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).
> 
> 
> 
> After the return of ioctl with stop != 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 stop == 0, the device can continue
> 
> processing buffers as long as typical conditions are met (vq is enabled,
> 
> DRIVER_OK status bit is enabled, etc).

Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.

Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?
This is in the context of other discussion we had in the LM series.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 1/4] vdpa: Add stop operation

[Stefano Garzarella]

On Thu, May 26, 2022 at 02:43: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 | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>index 15af802d41c4..ddfebc4e1e01 100644
>--- a/include/linux/vdpa.h
>+++ b/include/linux/vdpa.h
>@@ -215,6 +215,11 @@ struct vdpa_map_file {
>  * @reset:			Reset device
>  *				@vdev: vdpa device
>  *				Returns integer: success (0) or error (< 0)
>+ * @stop:			Stop or resume the device (optional, but it must
>+ *				be implemented if require device stop)
>+ *				@vdev: vdpa device
>+ *				@stop: stop (true), not stop (false)

Sorry for just seeing this now, but if you have to send a v5, maybe we 
could use "resume" here instead of "not stop".

Thanks,
Stefano

>+ *				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
>@@ -316,6 +321,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 (*stop)(struct vdpa_device *vdev, bool stop);
> 	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 v4 1/4] vdpa: Add stop operation

[Stefano Garzarella]

On Thu, May 26, 2022 at 02:43: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 | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>index 15af802d41c4..ddfebc4e1e01 100644
>--- a/include/linux/vdpa.h
>+++ b/include/linux/vdpa.h
>@@ -215,6 +215,11 @@ struct vdpa_map_file {
>  * @reset:			Reset device
>  *				@vdev: vdpa device
>  *				Returns integer: success (0) or error (< 0)
>+ * @stop:			Stop or resume the device (optional, but it must
>+ *				be implemented if require device stop)
>+ *				@vdev: vdpa device
>+ *				@stop: stop (true), not stop (false)

Sorry for just seeing this now, but if you have to send a v5, maybe we 
could use "resume" here instead of "not stop".

Thanks,
Stefano

>+ *				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
>@@ -316,6 +321,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 (*stop)(struct vdpa_device *vdev, bool stop);
> 	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 v4 4/4] vdpa_sim: Implement stop vdpa op

[Stefano Garzarella]

On Thu, May 26, 2022 at 02:43:38PM +0200, Eugenio Pérez wrote:
>Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
>that backend feature and userspace can effectively stop 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).
>
>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(+)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>index 50d721072beb..0515cf314bed 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_stop(struct vdpa_device *vdpa, bool stop)
>+{
>+	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>+	int i;
>+
>+	spin_lock(&vdpasim->lock);
>+	vdpasim->running = !stop;
>+	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,
>+	.stop			= vdpasim_stop,
> 	.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,
>+	.stop			= vdpasim_stop,
> 	.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 v4 4/4] vdpa_sim: Implement stop vdpa op

[Stefano Garzarella]

On Thu, May 26, 2022 at 02:43:38PM +0200, Eugenio Pérez wrote:
>Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
>that backend feature and userspace can effectively stop 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).
>
>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(+)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>index 50d721072beb..0515cf314bed 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_stop(struct vdpa_device *vdpa, bool stop)
>+{
>+	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
>+	int i;
>+
>+	spin_lock(&vdpasim->lock);
>+	vdpasim->running = !stop;
>+	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,
>+	.stop			= vdpasim_stop,
> 	.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,
>+	.stop			= vdpasim_stop,
> 	.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 v4 1/4] vdpa: Add stop operation

[Eugenio Perez Martin]

On Thu, May 26, 2022 at 4:23 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, May 26, 2022 at 02:43: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 | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> >diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> >index 15af802d41c4..ddfebc4e1e01 100644
> >--- a/include/linux/vdpa.h
> >+++ b/include/linux/vdpa.h
> >@@ -215,6 +215,11 @@ struct vdpa_map_file {
> >  * @reset:                    Reset device
> >  *                            @vdev: vdpa device
> >  *                            Returns integer: success (0) or error (< 0)
> >+ * @stop:                     Stop or resume the device (optional, but it must
> >+ *                            be implemented if require device stop)
> >+ *                            @vdev: vdpa device
> >+ *                            @stop: stop (true), not stop (false)
>
> Sorry for just seeing this now, but if you have to send a v5, maybe we
> could use "resume" here instead of "not stop".
>

I agree it fits way better, I'll queue for the next :). Thanks!

> Thanks,
> Stefano
>
> >+ *                            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
> >@@ -316,6 +321,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 (*stop)(struct vdpa_device *vdev, bool stop);
> >       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 v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Thu, May 26, 2022 at 8:54 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Eugenio Pérez <eperezma@redhat.com>
> > Sent: Thursday, May 26, 2022 8:44 AM
>
> > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> >
> > that backend feature and userspace can effectively stop 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).
> >
> >
> >
> > After the return of ioctl with stop != 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 stop == 0, the device can continue
> >
> > processing buffers as long as typical conditions are met (vq is enabled,
> >
> > DRIVER_OK status bit is enabled, etc).
>
> Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.

We try to provide forward compatibility to VIRTIO_CONFIG_S_STOP. That
means it is expected to implement at least a subset of
VIRTIO_CONFIG_S_STOP.

>
> Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?

So the idea is to add capability that does not exist in the spec. Then
came the stop/resume which can't be done via DRIVER_OK. I think we
should only allow the stop/resume to succeed after DRIVER_OK is set.

> This is in the context of other discussion we had in the LM series.

Do you see any issue that blocks the live migration?

Thanks

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Thu, May 26, 2022 at 8:54 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Eugenio Pérez <eperezma@redhat.com>
> > Sent: Thursday, May 26, 2022 8:44 AM
>
> > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> >
> > that backend feature and userspace can effectively stop 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).
> >
> >
> >
> > After the return of ioctl with stop != 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 stop == 0, the device can continue
> >
> > processing buffers as long as typical conditions are met (vq is enabled,
> >
> > DRIVER_OK status bit is enabled, etc).
>
> Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.

We try to provide forward compatibility to VIRTIO_CONFIG_S_STOP. That
means it is expected to implement at least a subset of
VIRTIO_CONFIG_S_STOP.

>
> Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?

So the idea is to add capability that does not exist in the spec. Then
came the stop/resume which can't be done via DRIVER_OK. I think we
should only allow the stop/resume to succeed after DRIVER_OK is set.

> This is in the context of other discussion we had in the LM series.

Do you see any issue that blocks the live migration?

Thanks

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

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Eugenio Perez Martin]

On Fri, May 27, 2022 at 4:26 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, May 26, 2022 at 8:54 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> >
> > > From: Eugenio Pérez <eperezma@redhat.com>
> > > Sent: Thursday, May 26, 2022 8:44 AM
> >
> > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > >
> > > that backend feature and userspace can effectively stop 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).
> > >
> > >
> > >
> > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > >
> > > processing buffers as long as typical conditions are met (vq is enabled,
> > >
> > > DRIVER_OK status bit is enabled, etc).
> >
> > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.
>
> We try to provide forward compatibility to VIRTIO_CONFIG_S_STOP. That
> means it is expected to implement at least a subset of
> VIRTIO_CONFIG_S_STOP.
>

Appending a link to the proposal, just for reference [1].

> >
> > Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?
>

Parav, I'm not sure I follow you here.

By the proposal, the resume of the device is (From qemu POV):
1. To configure all data vqs and cvq (addr, num, ...)
2. To enable only CVQ, not data vqs
3. To send DRIVER_OK
4. Wait for all buffers of CVQ to be used
5. To enable all others data vqs (individual ioctl at the moment)

Where can we fit the resume (as "stop(false)") here? If the device is
stopped (as if we send stop(true) before DRIVER_OK), we don't read CVQ
first. If we send it right after (or instead) DRIVER_OK, data buffers
can reach data vqs before configuring RSS.

> So the idea is to add capability that does not exist in the spec. Then
> came the stop/resume which can't be done via DRIVER_OK. I think we
> should only allow the stop/resume to succeed after DRIVER_OK is set.
>
> > This is in the context of other discussion we had in the LM series.
>
> Do you see any issue that blocks the live migration?
>
> Thanks
>

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Michael S. Tsirkin]

On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> 
> 
> > From: Eugenio Pérez <eperezma@redhat.com>
> > Sent: Thursday, May 26, 2022 8:44 AM
> 
> > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > 
> > that backend feature and userspace can effectively stop 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).
> > 
> > 
> > 
> > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > 
> > processing buffers as long as typical conditions are met (vq is enabled,
> > 
> > DRIVER_OK status bit is enabled, etc).
> 
> Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.
> 
> Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?
> This is in the context of other discussion we had in the LM series.

If there's something in the spec that does this then let's use that.
Unfortunately the LM series seems to be stuck on moving
bits around with the admin virtqueue ...

-- 
MST

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Michael S. Tsirkin]

On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> 
> 
> > From: Eugenio Pérez <eperezma@redhat.com>
> > Sent: Thursday, May 26, 2022 8:44 AM
> 
> > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > 
> > that backend feature and userspace can effectively stop 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).
> > 
> > 
> > 
> > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > 
> > processing buffers as long as typical conditions are met (vq is enabled,
> > 
> > DRIVER_OK status bit is enabled, etc).
> 
> Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.
> 
> Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?
> This is in the context of other discussion we had in the LM series.

If there's something in the spec that does this then let's use that.
Unfortunately the LM series seems to be stuck on moving
bits around with the admin virtqueue ...

-- 
MST

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

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Eugenio Pérez <eperezma@redhat.com>
> > > Sent: Thursday, May 26, 2022 8:44 AM
> >
> > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > >
> > > that backend feature and userspace can effectively stop 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).
> > >
> > >
> > >
> > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > >
> > > processing buffers as long as typical conditions are met (vq is enabled,
> > >
> > > DRIVER_OK status bit is enabled, etc).
> >
> > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.
> >
> > Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?
> > This is in the context of other discussion we had in the LM series.
>
> If there's something in the spec that does this then let's use that.

Actually, we try to propose a independent feature here:

https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html

Does it make sense to you?

Thanks

> Unfortunately the LM series seems to be stuck on moving
> bits around with the admin virtqueue ...
>
> --
> MST
>

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Eugenio Pérez <eperezma@redhat.com>
> > > Sent: Thursday, May 26, 2022 8:44 AM
> >
> > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > >
> > > that backend feature and userspace can effectively stop 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).
> > >
> > >
> > >
> > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > >
> > > processing buffers as long as typical conditions are met (vq is enabled,
> > >
> > > DRIVER_OK status bit is enabled, etc).
> >
> > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.
> >
> > Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?
> > This is in the context of other discussion we had in the LM series.
>
> If there's something in the spec that does this then let's use that.

Actually, we try to propose a independent feature here:

https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html

Does it make sense to you?

Thanks

> Unfortunately the LM series seems to be stuck on moving
> bits around with the admin virtqueue ...
>
> --
> MST
>

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

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Michael S. Tsirkin]

On Mon, May 30, 2022 at 11:39:21AM +0800, Jason Wang wrote:
> On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > Sent: Thursday, May 26, 2022 8:44 AM
> > >
> > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > > >
> > > > that backend feature and userspace can effectively stop 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).
> > > >
> > > >
> > > >
> > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > >
> > > > processing buffers as long as typical conditions are met (vq is enabled,
> > > >
> > > > DRIVER_OK status bit is enabled, etc).
> > >
> > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.
> > >
> > > Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?
> > > This is in the context of other discussion we had in the LM series.
> >
> > If there's something in the spec that does this then let's use that.
> 
> Actually, we try to propose a independent feature here:
> 
> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> 
> Does it make sense to you?
> 
> Thanks

But I thought the LM patches are trying to replace all that?


> > Unfortunately the LM series seems to be stuck on moving
> > bits around with the admin virtqueue ...
> >
> > --
> > MST
> >

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Michael S. Tsirkin]

On Mon, May 30, 2022 at 11:39:21AM +0800, Jason Wang wrote:
> On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > Sent: Thursday, May 26, 2022 8:44 AM
> > >
> > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > > >
> > > > that backend feature and userspace can effectively stop 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).
> > > >
> > > >
> > > >
> > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > >
> > > > processing buffers as long as typical conditions are met (vq is enabled,
> > > >
> > > > DRIVER_OK status bit is enabled, etc).
> > >
> > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.
> > >
> > > Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?
> > > This is in the context of other discussion we had in the LM series.
> >
> > If there's something in the spec that does this then let's use that.
> 
> Actually, we try to propose a independent feature here:
> 
> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> 
> Does it make sense to you?
> 
> Thanks

But I thought the LM patches are trying to replace all that?


> > Unfortunately the LM series seems to be stuck on moving
> > bits around with the admin virtqueue ...
> >
> > --
> > MST
> >

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

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Michael S. Tsirkin]

On Thu, May 26, 2022 at 02:43:34PM +0200, Eugenio Pérez wrote:
> Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> that backend feature and userspace can effectively stop 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).
> 
> After the return of ioctl with stop != 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 stop == 0, the device can continue
> processing buffers as long as typical conditions are met (vq is enabled,
> DRIVER_OK status bit is enabled, etc).
> 
> In the future, we will provide features similar to VHOST_USER_GET_INFLIGHT_FD
> so the device can save pending operations.
> 
> Comments are welcome.


So given this is just for simulator and affects UAPI I think it's fine
to make it wait for the next merge window, until there's a consensus.
Right?

> 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 stop operation
>   vhost-vdpa: introduce STOP backend feature bit
>   vhost-vdpa: uAPI to stop the device
>   vdpa_sim: Implement stop 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                 | 34 +++++++++++++++++++++++++++-
>  include/linux/vdpa.h                 |  6 +++++
>  include/uapi/linux/vhost.h           | 14 ++++++++++++
>  include/uapi/linux/vhost_types.h     |  2 ++
>  8 files changed, 83 insertions(+), 1 deletion(-)
> 
> -- 
> 2.31.1
> 

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Michael S. Tsirkin]

On Thu, May 26, 2022 at 02:43:34PM +0200, Eugenio Pérez wrote:
> Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> that backend feature and userspace can effectively stop 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).
> 
> After the return of ioctl with stop != 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 stop == 0, the device can continue
> processing buffers as long as typical conditions are met (vq is enabled,
> DRIVER_OK status bit is enabled, etc).
> 
> In the future, we will provide features similar to VHOST_USER_GET_INFLIGHT_FD
> so the device can save pending operations.
> 
> Comments are welcome.


So given this is just for simulator and affects UAPI I think it's fine
to make it wait for the next merge window, until there's a consensus.
Right?

> 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 stop operation
>   vhost-vdpa: introduce STOP backend feature bit
>   vhost-vdpa: uAPI to stop the device
>   vdpa_sim: Implement stop 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                 | 34 +++++++++++++++++++++++++++-
>  include/linux/vdpa.h                 |  6 +++++
>  include/uapi/linux/vhost.h           | 14 ++++++++++++
>  include/uapi/linux/vhost_types.h     |  2 ++
>  8 files changed, 83 insertions(+), 1 deletion(-)
> 
> -- 
> 2.31.1
> 

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

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Tue, May 31, 2022 at 1:40 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, May 30, 2022 at 11:39:21AM +0800, Jason Wang wrote:
> > On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > > Sent: Thursday, May 26, 2022 8:44 AM
> > > >
> > > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > > > >
> > > > > that backend feature and userspace can effectively stop 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).
> > > > >
> > > > >
> > > > >
> > > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > > >
> > > > > processing buffers as long as typical conditions are met (vq is enabled,
> > > > >
> > > > > DRIVER_OK status bit is enabled, etc).
> > > >
> > > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.
> > > >
> > > > Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?
> > > > This is in the context of other discussion we had in the LM series.
> > >
> > > If there's something in the spec that does this then let's use that.
> >
> > Actually, we try to propose a independent feature here:
> >
> > https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> >
> > Does it make sense to you?
> >
> > Thanks
>
> But I thought the LM patches are trying to replace all that?

I'm not sure, and actually I think they are orthogonal. We need a new
state and the command to set the state could be transport specific or
a virtqueue.

As far as I know, most of the vendors have implemented this semantic.

Thanks

>
>
> > > Unfortunately the LM series seems to be stuck on moving
> > > bits around with the admin virtqueue ...
> > >
> > > --
> > > MST
> > >
>

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Tue, May 31, 2022 at 1:40 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, May 30, 2022 at 11:39:21AM +0800, Jason Wang wrote:
> > On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > > Sent: Thursday, May 26, 2022 8:44 AM
> > > >
> > > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > > > >
> > > > > that backend feature and userspace can effectively stop 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).
> > > > >
> > > > >
> > > > >
> > > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > > >
> > > > > processing buffers as long as typical conditions are met (vq is enabled,
> > > > >
> > > > > DRIVER_OK status bit is enabled, etc).
> > > >
> > > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to any mechanism in the virtio spec.
> > > >
> > > > Why can't we use this ioctl() to indicate driver to start/stop the device instead of driving it through the driver_ok?
> > > > This is in the context of other discussion we had in the LM series.
> > >
> > > If there's something in the spec that does this then let's use that.
> >
> > Actually, we try to propose a independent feature here:
> >
> > https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> >
> > Does it make sense to you?
> >
> > Thanks
>
> But I thought the LM patches are trying to replace all that?

I'm not sure, and actually I think they are orthogonal. We need a new
state and the command to set the state could be transport specific or
a virtqueue.

As far as I know, most of the vendors have implemented this semantic.

Thanks

>
>
> > > Unfortunately the LM series seems to be stuck on moving
> > > bits around with the admin virtqueue ...
> > >
> > > --
> > > MST
> > >
>

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

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Eugenio Perez Martin]

On Tue, May 31, 2022 at 7:42 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, May 26, 2022 at 02:43:34PM +0200, Eugenio Pérez wrote:
> > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > that backend feature and userspace can effectively stop 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).
> >
> > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > processing buffers as long as typical conditions are met (vq is enabled,
> > DRIVER_OK status bit is enabled, etc).
> >
> > In the future, we will provide features similar to VHOST_USER_GET_INFLIGHT_FD
> > so the device can save pending operations.
> >
> > Comments are welcome.
>
>
> So given this is just for simulator and affects UAPI I think it's fine
> to make it wait for the next merge window, until there's a consensus.
> Right?
>

While the change is only implemented in the simulator at this moment,
it's just the very last missing piece in the kernel to implement
complete live migration for net devices with cvq :). All vendor
drivers can implement this call with current code, just a little bit
of plumbing is needed. And it was accepted in previous meetings.

If it proves it works for every configuration (nested, etc), the
implementation can forward the call to the admin vq for example. At
the moment, it follows the proposed stop status bit sematic to stop
the device, which POC has been tested in these circumstances.

Thanks!

> > 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 stop operation
> >   vhost-vdpa: introduce STOP backend feature bit
> >   vhost-vdpa: uAPI to stop the device
> >   vdpa_sim: Implement stop 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                 | 34 +++++++++++++++++++++++++++-
> >  include/linux/vdpa.h                 |  6 +++++
> >  include/uapi/linux/vhost.h           | 14 ++++++++++++
> >  include/uapi/linux/vhost_types.h     |  2 ++
> >  8 files changed, 83 insertions(+), 1 deletion(-)
> >
> > --
> > 2.31.1
> >
>

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Michael S. Tsirkin]

On Tue, May 31, 2022 at 09:13:38AM +0200, Eugenio Perez Martin wrote:
> On Tue, May 31, 2022 at 7:42 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, May 26, 2022 at 02:43:34PM +0200, Eugenio Pérez wrote:
> > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > > that backend feature and userspace can effectively stop 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).
> > >
> > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > processing buffers as long as typical conditions are met (vq is enabled,
> > > DRIVER_OK status bit is enabled, etc).
> > >
> > > In the future, we will provide features similar to VHOST_USER_GET_INFLIGHT_FD
> > > so the device can save pending operations.
> > >
> > > Comments are welcome.
> >
> >
> > So given this is just for simulator and affects UAPI I think it's fine
> > to make it wait for the next merge window, until there's a consensus.
> > Right?
> >
> 
> While the change is only implemented in the simulator at this moment,
> it's just the very last missing piece in the kernel to implement
> complete live migration for net devices with cvq :). All vendor
> drivers can implement this call with current code, just a little bit
> of plumbing is needed. And it was accepted in previous meetings.
> 
> If it proves it works for every configuration (nested, etc), the
> implementation can forward the call to the admin vq for example. At
> the moment, it follows the proposed stop status bit sematic to stop
> the device, which POC has been tested in these circumstances.
> 
> Thanks!

Oh absolutely, but I am guessing this plumbing won't
be ready for this merge window.

> > > 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 stop operation
> > >   vhost-vdpa: introduce STOP backend feature bit
> > >   vhost-vdpa: uAPI to stop the device
> > >   vdpa_sim: Implement stop 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                 | 34 +++++++++++++++++++++++++++-
> > >  include/linux/vdpa.h                 |  6 +++++
> > >  include/uapi/linux/vhost.h           | 14 ++++++++++++
> > >  include/uapi/linux/vhost_types.h     |  2 ++
> > >  8 files changed, 83 insertions(+), 1 deletion(-)
> > >
> > > --
> > > 2.31.1
> > >
> >

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Michael S. Tsirkin]

On Tue, May 31, 2022 at 09:13:38AM +0200, Eugenio Perez Martin wrote:
> On Tue, May 31, 2022 at 7:42 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, May 26, 2022 at 02:43:34PM +0200, Eugenio Pérez wrote:
> > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> > > that backend feature and userspace can effectively stop 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).
> > >
> > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > processing buffers as long as typical conditions are met (vq is enabled,
> > > DRIVER_OK status bit is enabled, etc).
> > >
> > > In the future, we will provide features similar to VHOST_USER_GET_INFLIGHT_FD
> > > so the device can save pending operations.
> > >
> > > Comments are welcome.
> >
> >
> > So given this is just for simulator and affects UAPI I think it's fine
> > to make it wait for the next merge window, until there's a consensus.
> > Right?
> >
> 
> While the change is only implemented in the simulator at this moment,
> it's just the very last missing piece in the kernel to implement
> complete live migration for net devices with cvq :). All vendor
> drivers can implement this call with current code, just a little bit
> of plumbing is needed. And it was accepted in previous meetings.
> 
> If it proves it works for every configuration (nested, etc), the
> implementation can forward the call to the admin vq for example. At
> the moment, it follows the proposed stop status bit sematic to stop
> the device, which POC has been tested in these circumstances.
> 
> Thanks!

Oh absolutely, but I am guessing this plumbing won't
be ready for this merge window.

> > > 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 stop operation
> > >   vhost-vdpa: introduce STOP backend feature bit
> > >   vhost-vdpa: uAPI to stop the device
> > >   vdpa_sim: Implement stop 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                 | 34 +++++++++++++++++++++++++++-
> > >  include/linux/vdpa.h                 |  6 +++++
> > >  include/uapi/linux/vhost.h           | 14 ++++++++++++
> > >  include/uapi/linux/vhost_types.h     |  2 ++
> > >  8 files changed, 83 insertions(+), 1 deletion(-)
> > >
> > > --
> > > 2.31.1
> > >
> >

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

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Sunday, May 29, 2022 11:39 PM
> 
> On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > Sent: Thursday, May 26, 2022 8:44 AM
> > >
> > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will
> > > > offer
> > > >
> > > > that backend feature and userspace can effectively stop 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).
> > > >
> > > >
> > > >
> > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > >
> > > > processing buffers as long as typical conditions are met (vq is
> > > > enabled,
> > > >
> > > > DRIVER_OK status bit is enabled, etc).
> > >
> > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to
> any mechanism in the virtio spec.
> > >
> > > Why can't we use this ioctl() to indicate driver to start/stop the device
> instead of driving it through the driver_ok?
> > > This is in the context of other discussion we had in the LM series.
> >
> > If there's something in the spec that does this then let's use that.
> 
> Actually, we try to propose a independent feature here:
> 
> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> 
This will stop the device for all the operations.
Once the device is stopped, its state cannot be queried further as device won't respond.
It has limited use case.
What we need is to stop non admin queue related portion of the device.

> Does it make sense to you?
> 
> Thanks
> 
> > Unfortunately the LM series seems to be stuck on moving bits around
> > with the admin virtqueue ...
> >
> > --
> > MST
> >

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit via Virtualization]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Sunday, May 29, 2022 11:39 PM
> 
> On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > Sent: Thursday, May 26, 2022 8:44 AM
> > >
> > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will
> > > > offer
> > > >
> > > > that backend feature and userspace can effectively stop 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).
> > > >
> > > >
> > > >
> > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > >
> > > > processing buffers as long as typical conditions are met (vq is
> > > > enabled,
> > > >
> > > > DRIVER_OK status bit is enabled, etc).
> > >
> > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to
> any mechanism in the virtio spec.
> > >
> > > Why can't we use this ioctl() to indicate driver to start/stop the device
> instead of driving it through the driver_ok?
> > > This is in the context of other discussion we had in the LM series.
> >
> > If there's something in the spec that does this then let's use that.
> 
> Actually, we try to propose a independent feature here:
> 
> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> 
This will stop the device for all the operations.
Once the device is stopped, its state cannot be queried further as device won't respond.
It has limited use case.
What we need is to stop non admin queue related portion of the device.

> Does it make sense to you?
> 
> Thanks
> 
> > Unfortunately the LM series seems to be stuck on moving bits around
> > with the admin virtqueue ...
> >
> > --
> > MST
> >

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

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit]

> From: Eugenio Perez Martin <eperezma@redhat.com>
> Sent: Friday, May 27, 2022 3:55 AM
> 
> On Fri, May 27, 2022 at 4:26 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, May 26, 2022 at 8:54 PM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > >
> > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > Sent: Thursday, May 26, 2022 8:44 AM
> > >
> > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will
> > > > offer
> > > >
> > > > that backend feature and userspace can effectively stop 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).
> > > >
> > > >
> > > >
> > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > >
> > > > processing buffers as long as typical conditions are met (vq is
> > > > enabled,
> > > >
> > > > DRIVER_OK status bit is enabled, etc).
> > >
> > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to
> any mechanism in the virtio spec.
> >
> > We try to provide forward compatibility to VIRTIO_CONFIG_S_STOP. That
> > means it is expected to implement at least a subset of
> > VIRTIO_CONFIG_S_STOP.
> >
> 
> Appending a link to the proposal, just for reference [1].
> 
> > >
> > > Why can't we use this ioctl() to indicate driver to start/stop the device
> instead of driving it through the driver_ok?
> >
> 
> Parav, I'm not sure I follow you here.
> 
> By the proposal, the resume of the device is (From qemu POV):
> 1. To configure all data vqs and cvq (addr, num, ...) 2. To enable only CVQ, not
> data vqs 3. To send DRIVER_OK 4. Wait for all buffers of CVQ to be used 5. To
> enable all others data vqs (individual ioctl at the moment)
> 
> Where can we fit the resume (as "stop(false)") here? If the device is stopped
> (as if we send stop(true) before DRIVER_OK), we don't read CVQ first. If we
> send it right after (or instead) DRIVER_OK, data buffers can reach data vqs
> before configuring RSS.
> 
It doesn’t make sense with currently proposed way of using cvq to replay the config.
Need to continue with currently proposed temporary method that subsequently to be replaced with optimized flow as we discussed.

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit via Virtualization]

> From: Eugenio Perez Martin <eperezma@redhat.com>
> Sent: Friday, May 27, 2022 3:55 AM
> 
> On Fri, May 27, 2022 at 4:26 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, May 26, 2022 at 8:54 PM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > >
> > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > Sent: Thursday, May 26, 2022 8:44 AM
> > >
> > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will
> > > > offer
> > > >
> > > > that backend feature and userspace can effectively stop 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).
> > > >
> > > >
> > > >
> > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > >
> > > > processing buffers as long as typical conditions are met (vq is
> > > > enabled,
> > > >
> > > > DRIVER_OK status bit is enabled, etc).
> > >
> > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to
> any mechanism in the virtio spec.
> >
> > We try to provide forward compatibility to VIRTIO_CONFIG_S_STOP. That
> > means it is expected to implement at least a subset of
> > VIRTIO_CONFIG_S_STOP.
> >
> 
> Appending a link to the proposal, just for reference [1].
> 
> > >
> > > Why can't we use this ioctl() to indicate driver to start/stop the device
> instead of driving it through the driver_ok?
> >
> 
> Parav, I'm not sure I follow you here.
> 
> By the proposal, the resume of the device is (From qemu POV):
> 1. To configure all data vqs and cvq (addr, num, ...) 2. To enable only CVQ, not
> data vqs 3. To send DRIVER_OK 4. Wait for all buffers of CVQ to be used 5. To
> enable all others data vqs (individual ioctl at the moment)
> 
> Where can we fit the resume (as "stop(false)") here? If the device is stopped
> (as if we send stop(true) before DRIVER_OK), we don't read CVQ first. If we
> send it right after (or instead) DRIVER_OK, data buffers can reach data vqs
> before configuring RSS.
> 
It doesn’t make sense with currently proposed way of using cvq to replay the config.
Need to continue with currently proposed temporary method that subsequently to be replaced with optimized flow as we discussed.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Wed, Jun 1, 2022 at 4:19 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Sunday, May 29, 2022 11:39 PM
> >
> > On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > > Sent: Thursday, May 26, 2022 8:44 AM
> > > >
> > > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will
> > > > > offer
> > > > >
> > > > > that backend feature and userspace can effectively stop 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).
> > > > >
> > > > >
> > > > >
> > > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > > >
> > > > > processing buffers as long as typical conditions are met (vq is
> > > > > enabled,
> > > > >
> > > > > DRIVER_OK status bit is enabled, etc).
> > > >
> > > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to
> > any mechanism in the virtio spec.
> > > >
> > > > Why can't we use this ioctl() to indicate driver to start/stop the device
> > instead of driving it through the driver_ok?
> > > > This is in the context of other discussion we had in the LM series.
> > >
> > > If there's something in the spec that does this then let's use that.
> >
> > Actually, we try to propose a independent feature here:
> >
> > https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> >
> This will stop the device for all the operations.

Well, the ability to query the virtqueue state was proposed as another
feature (Eugenio, please correct me). This should be sufficient for
making virtio-net to be live migrated.

https://lists.oasis-open.org/archives/virtio-comment/202103/msg00008.html

> Once the device is stopped, its state cannot be queried further as device won't respond.
> It has limited use case.
> What we need is to stop non admin queue related portion of the device.

See above.

Thanks

>
> > Does it make sense to you?
> >
> > Thanks
> >
> > > Unfortunately the LM series seems to be stuck on moving bits around
> > > with the admin virtqueue ...
> > >
> > > --
> > > MST
> > >
>

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

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Wed, Jun 1, 2022 at 4:19 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Sunday, May 29, 2022 11:39 PM
> >
> > On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > > Sent: Thursday, May 26, 2022 8:44 AM
> > > >
> > > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will
> > > > > offer
> > > > >
> > > > > that backend feature and userspace can effectively stop 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).
> > > > >
> > > > >
> > > > >
> > > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > > >
> > > > > processing buffers as long as typical conditions are met (vq is
> > > > > enabled,
> > > > >
> > > > > DRIVER_OK status bit is enabled, etc).
> > > >
> > > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to
> > any mechanism in the virtio spec.
> > > >
> > > > Why can't we use this ioctl() to indicate driver to start/stop the device
> > instead of driving it through the driver_ok?
> > > > This is in the context of other discussion we had in the LM series.
> > >
> > > If there's something in the spec that does this then let's use that.
> >
> > Actually, we try to propose a independent feature here:
> >
> > https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> >
> This will stop the device for all the operations.

Well, the ability to query the virtqueue state was proposed as another
feature (Eugenio, please correct me). This should be sufficient for
making virtio-net to be live migrated.

https://lists.oasis-open.org/archives/virtio-comment/202103/msg00008.html

> Once the device is stopped, its state cannot be queried further as device won't respond.
> It has limited use case.
> What we need is to stop non admin queue related portion of the device.

See above.

Thanks

>
> > Does it make sense to you?
> >
> > Thanks
> >
> > > Unfortunately the LM series seems to be stuck on moving bits around
> > > with the admin virtqueue ...
> > >
> > > --
> > > MST
> > >
>

RE: [PATCH v4 1/4] vdpa: Add stop operation

[Eli Cohen]

> From: Eugenio Pérez <eperezma@redhat.com>
> Sent: Thursday, May 26, 2022 3:44 PM
> To: Michael S. Tsirkin <mst@redhat.com>; kvm@vger.kernel.org; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org;
> Jason Wang <jasowang@redhat.com>; netdev@vger.kernel.org
> Cc: martinh@xilinx.com; Stefano Garzarella <sgarzare@redhat.com>; martinpo@xilinx.com; lvivier@redhat.com; pabloc@xilinx.com;
> Parav Pandit <parav@nvidia.com>; Eli Cohen <elic@nvidia.com>; Dan Carpenter <dan.carpenter@oracle.com>; Xie Yongji
> <xieyongji@bytedance.com>; Christophe JAILLET <christophe.jaillet@wanadoo.fr>; Zhang Min <zhang.min9@zte.com.cn>; Wu Zongyong
> <wuzongyong@linux.alibaba.com>; lulu@redhat.com; Zhu Lingshan <lingshan.zhu@intel.com>; Piotr.Uminski@intel.com; Si-Wei Liu <si-
> wei.liu@oracle.com>; ecree.xilinx@gmail.com; gautam.dawar@amd.com; habetsm.xilinx@gmail.com; tanuj.kamde@amd.com;
> hanand@xilinx.com; dinang@xilinx.com; Longpeng <longpeng2@huawei.com>
> Subject: [PATCH v4 1/4] vdpa: Add stop operation
> 
> 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 15af802d41c4..ddfebc4e1e01 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -215,6 +215,11 @@ struct vdpa_map_file {
>   * @reset:			Reset device
>   *				@vdev: vdpa device
>   *				Returns integer: success (0) or error (< 0)
> + * @stop:			Stop or resume the device (optional, but it must
> + *				be implemented if require device stop)
> + *				@vdev: vdpa device
> + *				@stop: stop (true), not stop (false)
> + *				Returns integer: success (0) or error (< 0)

I assume after successful "stop" the device is guaranteed to stop processing descriptors and after resume it may process descriptors?
If that is so, I think it should be clear in the change log.

>   * @get_config_size:		Get the size of the configuration space includes
>   *				fields that are conditional on feature bits.
>   *				@vdev: vdpa device
> @@ -316,6 +321,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 (*stop)(struct vdpa_device *vdev, bool stop);
>  	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 v4 1/4] vdpa: Add stop operation

[Eugenio Perez Martin]

On Wed, Jun 1, 2022 at 7:35 AM Eli Cohen <elic@nvidia.com> wrote:
>
> > From: Eugenio Pérez <eperezma@redhat.com>
> > Sent: Thursday, May 26, 2022 3:44 PM
> > To: Michael S. Tsirkin <mst@redhat.com>; kvm@vger.kernel.org; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org;
> > Jason Wang <jasowang@redhat.com>; netdev@vger.kernel.org
> > Cc: martinh@xilinx.com; Stefano Garzarella <sgarzare@redhat.com>; martinpo@xilinx.com; lvivier@redhat.com; pabloc@xilinx.com;
> > Parav Pandit <parav@nvidia.com>; Eli Cohen <elic@nvidia.com>; Dan Carpenter <dan.carpenter@oracle.com>; Xie Yongji
> > <xieyongji@bytedance.com>; Christophe JAILLET <christophe.jaillet@wanadoo.fr>; Zhang Min <zhang.min9@zte.com.cn>; Wu Zongyong
> > <wuzongyong@linux.alibaba.com>; lulu@redhat.com; Zhu Lingshan <lingshan.zhu@intel.com>; Piotr.Uminski@intel.com; Si-Wei Liu <si-
> > wei.liu@oracle.com>; ecree.xilinx@gmail.com; gautam.dawar@amd.com; habetsm.xilinx@gmail.com; tanuj.kamde@amd.com;
> > hanand@xilinx.com; dinang@xilinx.com; Longpeng <longpeng2@huawei.com>
> > Subject: [PATCH v4 1/4] vdpa: Add stop operation
> >
> > 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 | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > index 15af802d41c4..ddfebc4e1e01 100644
> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -215,6 +215,11 @@ struct vdpa_map_file {
> >   * @reset:                   Reset device
> >   *                           @vdev: vdpa device
> >   *                           Returns integer: success (0) or error (< 0)
> > + * @stop:                    Stop or resume the device (optional, but it must
> > + *                           be implemented if require device stop)
> > + *                           @vdev: vdpa device
> > + *                           @stop: stop (true), not stop (false)
> > + *                           Returns integer: success (0) or error (< 0)
>
> I assume after successful "stop" the device is guaranteed to stop processing descriptors and after resume it may process descriptors?
> If that is so, I think it should be clear in the change log.
>

Yes.

It's better described in the changelog of vdpa sim change, maybe it's
better to move here.

Thanks!

> >   * @get_config_size:         Get the size of the configuration space includes
> >   *                           fields that are conditional on feature bits.
> >   *                           @vdev: vdpa device
> > @@ -316,6 +321,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 (*stop)(struct vdpa_device *vdev, bool stop);
> >       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 v4 0/4] Implement vdpasim stop operation

[Eugenio Perez Martin]

On Tue, May 31, 2022 at 10:19 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Sunday, May 29, 2022 11:39 PM
> >
> > On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > > Sent: Thursday, May 26, 2022 8:44 AM
> > > >
> > > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will
> > > > > offer
> > > > >
> > > > > that backend feature and userspace can effectively stop 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).
> > > > >
> > > > >
> > > > >
> > > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > > >
> > > > > processing buffers as long as typical conditions are met (vq is
> > > > > enabled,
> > > > >
> > > > > DRIVER_OK status bit is enabled, etc).
> > > >
> > > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to
> > any mechanism in the virtio spec.
> > > >
> > > > Why can't we use this ioctl() to indicate driver to start/stop the device
> > instead of driving it through the driver_ok?
> > > > This is in the context of other discussion we had in the LM series.
> > >
> > > If there's something in the spec that does this then let's use that.
> >
> > Actually, we try to propose a independent feature here:
> >
> > https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> >
> This will stop the device for all the operations.
> Once the device is stopped, its state cannot be queried further as device won't respond.
> It has limited use case.
> What we need is to stop non admin queue related portion of the device.
>

Still don't follow this, sorry.

Adding the admin vq to the mix, this would stop a device of a device
group, but not the whole virtqueue group. If the admin VQ is offered
by the PF (since it's not exposed to the guest), it will continue
accepting requests as normal. If it's exposed in the VF, I think the
best bet is to shadow it, since guest and host requests could
conflict.

Since this is offered through vdpa, the device backend driver can
route it to whatever method works better for the hardware. For
example, to send an admin vq command to the PF. That's why it's
important to keep the feature as self-contained and orthogonal to
others as possible.

> > Does it make sense to you?
> >
> > Thanks
> >
> > > Unfortunately the LM series seems to be stuck on moving bits around
> > > with the admin virtqueue ...
> > >
> > > --
> > > MST
> > >
>

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Eugenio Perez Martin]

On Tue, May 31, 2022 at 10:26 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Eugenio Perez Martin <eperezma@redhat.com>
> > Sent: Friday, May 27, 2022 3:55 AM
> >
> > On Fri, May 27, 2022 at 4:26 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, May 26, 2022 at 8:54 PM Parav Pandit <parav@nvidia.com> wrote:
> > > >
> > > >
> > > >
> > > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > > Sent: Thursday, May 26, 2022 8:44 AM
> > > >
> > > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa will
> > > > > offer
> > > > >
> > > > > that backend feature and userspace can effectively stop 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).
> > > > >
> > > > >
> > > > >
> > > > > After the return of ioctl with stop != 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 stop == 0, the device can continue
> > > > >
> > > > > processing buffers as long as typical conditions are met (vq is
> > > > > enabled,
> > > > >
> > > > > DRIVER_OK status bit is enabled, etc).
> > > >
> > > > Just to be clear, we are adding vdpa level new ioctl() that doesn’t map to
> > any mechanism in the virtio spec.
> > >
> > > We try to provide forward compatibility to VIRTIO_CONFIG_S_STOP. That
> > > means it is expected to implement at least a subset of
> > > VIRTIO_CONFIG_S_STOP.
> > >
> >
> > Appending a link to the proposal, just for reference [1].
> >
> > > >
> > > > Why can't we use this ioctl() to indicate driver to start/stop the device
> > instead of driving it through the driver_ok?
> > >
> >
> > Parav, I'm not sure I follow you here.
> >
> > By the proposal, the resume of the device is (From qemu POV):
> > 1. To configure all data vqs and cvq (addr, num, ...) 2. To enable only CVQ, not
> > data vqs 3. To send DRIVER_OK 4. Wait for all buffers of CVQ to be used 5. To
> > enable all others data vqs (individual ioctl at the moment)
> >
> > Where can we fit the resume (as "stop(false)") here? If the device is stopped
> > (as if we send stop(true) before DRIVER_OK), we don't read CVQ first. If we
> > send it right after (or instead) DRIVER_OK, data buffers can reach data vqs
> > before configuring RSS.
> >
> It doesn’t make sense with currently proposed way of using cvq to replay the config.

The stop/resume part is not intended to restore the config through the
CVQ. The stop call is issued to be able to retrieve the vq status
(base, in vhost terminology). The symmetric operation (resume) was
added on demand, it was never intended to be part neither of the
config restore or the virtqueue state restore workflow.

The configuration restore workflow was modelled after the device
initialization, so each part needed to add the less things the better,
and only qemu needed to be changed. From the device POV, there is no
need to learn new tricks for this. The support of .set_vq_ready and
.get_vq_ready is already in the kernel in every vdpa backend driver.

> Need to continue with currently proposed temporary method that subsequently to be replaced with optimized flow as we discussed.

Back then, it was noted by you that enabling each data vq individually
after DRIVER_OK is slow on mlx5 devices. The solution was to batch
these enable calls accounting in the kernel, achieving no growth in
the vdpa uAPI layer. The proposed solution did not involve the resume
operation.

After that, you proposed in this thread "Why can't we use this ioctl()
to indicate driver to start/stop the device instead of driving it
through the driver_ok?". As I understand, that is a mistake, since it
requires the device, the vdpa layer, etc... to learn new tricks. It
requires qemu to duplicate the initialization layer (it's now common
for start and restore config). But I might have not seen the whole
picture, missing advantages of using the resume call for this
workflow. Can you describe the workflow you have in mind? How does
that new workflow affect this proposal?

I'm ok to change the proposal as long as we find we obtain a net gain.

Thanks!

Re: [PATCH v4 3/4] vhost-vdpa: uAPI to stop the device

[Michael S. Tsirkin]

On Thu, May 26, 2022 at 02:43:37PM +0200, Eugenio Pérez wrote:
> The ioctl adds support for stop the device from userspace.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  drivers/vhost/vdpa.c       | 18 ++++++++++++++++++
>  include/uapi/linux/vhost.h | 14 ++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 32713db5831d..d1d19555c4b7 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
>  	return 0;
>  }
>  
> +static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
> +{
> +	struct vdpa_device *vdpa = v->vdpa;
> +	const struct vdpa_config_ops *ops = vdpa->config;
> +	int stop;
> +
> +	if (!ops->stop)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&stop, argp, sizeof(stop)))
> +		return -EFAULT;
> +
> +	return ops->stop(vdpa, stop);
> +}
> +
>  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>  				   void __user *argp)
>  {
> @@ -650,6 +665,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_STOP:
> +		r = vhost_vdpa_stop(v, argp);
> +		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..c7e47b29bf61 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)
>  
> +/* Stop or resume a device so it does not process virtqueue requests anymore
> + *
> + * After the return of ioctl with stop != 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 stop == 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_STOP			_IOW(VHOST_VIRTIO, 0x7D, int)
> +
>  #endif

I wonder how does this interact with the admin vq idea.
I.e. if we stop all VQs then apparently admin vq can't
work either ...
Thoughts?

> -- 
> 2.31.1

Re: [PATCH v4 3/4] vhost-vdpa: uAPI to stop the device

[Michael S. Tsirkin]

On Thu, May 26, 2022 at 02:43:37PM +0200, Eugenio Pérez wrote:
> The ioctl adds support for stop the device from userspace.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  drivers/vhost/vdpa.c       | 18 ++++++++++++++++++
>  include/uapi/linux/vhost.h | 14 ++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 32713db5831d..d1d19555c4b7 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
>  	return 0;
>  }
>  
> +static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
> +{
> +	struct vdpa_device *vdpa = v->vdpa;
> +	const struct vdpa_config_ops *ops = vdpa->config;
> +	int stop;
> +
> +	if (!ops->stop)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&stop, argp, sizeof(stop)))
> +		return -EFAULT;
> +
> +	return ops->stop(vdpa, stop);
> +}
> +
>  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>  				   void __user *argp)
>  {
> @@ -650,6 +665,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_STOP:
> +		r = vhost_vdpa_stop(v, argp);
> +		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..c7e47b29bf61 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)
>  
> +/* Stop or resume a device so it does not process virtqueue requests anymore
> + *
> + * After the return of ioctl with stop != 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 stop == 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_STOP			_IOW(VHOST_VIRTIO, 0x7D, int)
> +
>  #endif

I wonder how does this interact with the admin vq idea.
I.e. if we stop all VQs then apparently admin vq can't
work either ...
Thoughts?

> -- 
> 2.31.1

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

Re: [PATCH v4 3/4] vhost-vdpa: uAPI to stop the device

[Eugenio Perez Martin]

On Wed, Jun 1, 2022 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, May 26, 2022 at 02:43:37PM +0200, Eugenio Pérez wrote:
> > The ioctl adds support for stop the device from userspace.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  drivers/vhost/vdpa.c       | 18 ++++++++++++++++++
> >  include/uapi/linux/vhost.h | 14 ++++++++++++++
> >  2 files changed, 32 insertions(+)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index 32713db5831d..d1d19555c4b7 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
> >       return 0;
> >  }
> >
> > +static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
> > +{
> > +     struct vdpa_device *vdpa = v->vdpa;
> > +     const struct vdpa_config_ops *ops = vdpa->config;
> > +     int stop;
> > +
> > +     if (!ops->stop)
> > +             return -EOPNOTSUPP;
> > +
> > +     if (copy_from_user(&stop, argp, sizeof(stop)))
> > +             return -EFAULT;
> > +
> > +     return ops->stop(vdpa, stop);
> > +}
> > +
> >  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
> >                                  void __user *argp)
> >  {
> > @@ -650,6 +665,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_STOP:
> > +             r = vhost_vdpa_stop(v, argp);
> > +             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..c7e47b29bf61 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)
> >
> > +/* Stop or resume a device so it does not process virtqueue requests anymore
> > + *
> > + * After the return of ioctl with stop != 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 stop == 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_STOP                      _IOW(VHOST_VIRTIO, 0x7D, int)
> > +
> >  #endif
>
> I wonder how does this interact with the admin vq idea.
> I.e. if we stop all VQs then apparently admin vq can't
> work either ...
> Thoughts?
>

Copying here the answer to Parav, feel free to answer to any thread or
highlight if I missed something :). Using the admin vq proposal
terminology of "device group".

--
This would stop a device of a device
group, but not the whole virtqueue group. If the admin VQ is offered
by the PF (since it's not exposed to the guest), it will continue
accepting requests as normal. If it's exposed in the VF, I think the
best bet is to shadow it, since guest and host requests could
conflict.

Since this is offered through vdpa, the device backend driver can
route it to whatever method works better for the hardware. For
example, to send an admin vq command to the PF. That's why it's
important to keep the feature as self-contained and orthogonal to
others as possible.
--

> > --
> > 2.31.1
>

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit via Virtualization]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, May 31, 2022 10:42 PM
> 
> Well, the ability to query the virtqueue state was proposed as another
> feature (Eugenio, please correct me). This should be sufficient for making
> virtio-net to be live migrated.
> 
The device is stopped, it won't answer to this special vq config done here.
Programming all of these using cfg registers doesn't scale for on-chip memory and for the speed.

Next would be to program hundreds of statistics of the 64 VQs through giant PCI config space register in some busy polling scheme.

I can clearly see how all these are inefficient for faster LM.
We need an efficient AQ to proceed with at minimum.

> https://lists.oasis-open.org/archives/virtio-comment/202103/msg00008.html
> 
> > Once the device is stopped, its state cannot be queried further as device
> won't respond.
> > It has limited use case.
> > What we need is to stop non admin queue related portion of the device.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, May 31, 2022 10:42 PM
> 
> Well, the ability to query the virtqueue state was proposed as another
> feature (Eugenio, please correct me). This should be sufficient for making
> virtio-net to be live migrated.
> 
The device is stopped, it won't answer to this special vq config done here.
Programming all of these using cfg registers doesn't scale for on-chip memory and for the speed.

Next would be to program hundreds of statistics of the 64 VQs through giant PCI config space register in some busy polling scheme.

I can clearly see how all these are inefficient for faster LM.
We need an efficient AQ to proceed with at minimum.

> https://lists.oasis-open.org/archives/virtio-comment/202103/msg00008.html
> 
> > Once the device is stopped, its state cannot be queried further as device
> won't respond.
> > It has limited use case.
> > What we need is to stop non admin queue related portion of the device.

RE: [PATCH v4 3/4] vhost-vdpa: uAPI to stop the device

[Parav Pandit via Virtualization]

> From: Eugenio Perez Martin <eperezma@redhat.com>
> Sent: Wednesday, June 1, 2022 7:15 AM
> 
> On Wed, Jun 1, 2022 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, May 26, 2022 at 02:43:37PM +0200, Eugenio Pérez wrote:
> > > The ioctl adds support for stop the device from userspace.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  drivers/vhost/vdpa.c       | 18 ++++++++++++++++++
> > >  include/uapi/linux/vhost.h | 14 ++++++++++++++
> > >  2 files changed, 32 insertions(+)
> > >
> > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index
> > > 32713db5831d..d1d19555c4b7 100644
> > > --- a/drivers/vhost/vdpa.c
> > > +++ b/drivers/vhost/vdpa.c
> > > @@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct
> vhost_vdpa *v, u32 __user *argp)
> > >       return 0;
> > >  }
> > >
> > > +static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
> > > +{
> > > +     struct vdpa_device *vdpa = v->vdpa;
> > > +     const struct vdpa_config_ops *ops = vdpa->config;
> > > +     int stop;
> > > +
> > > +     if (!ops->stop)
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     if (copy_from_user(&stop, argp, sizeof(stop)))
> > > +             return -EFAULT;
> > > +
> > > +     return ops->stop(vdpa, stop);
> > > +}
> > > +
> > >  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int
> cmd,
> > >                                  void __user *argp)  { @@ -650,6
> > > +665,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_STOP:
> > > +             r = vhost_vdpa_stop(v, argp);
> > > +             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..c7e47b29bf61 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)
> > >
> > > +/* Stop or resume a device so it does not process virtqueue
> > > +requests anymore
> > > + *
> > > + * After the return of ioctl with stop != 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 stop == 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_STOP                      _IOW(VHOST_VIRTIO, 0x7D, int)
> > > +
A better name is VHOST_VDPA_SET_STATE
State = stop/suspend
State = start/resume

Suspend/resume seems more logical, as opposed start/stop, because it more clearly indicates that the resume (start) is from some programmed beginning (and not first boot).

> > >  #endif
> >
> > I wonder how does this interact with the admin vq idea.
> > I.e. if we stop all VQs then apparently admin vq can't work either ...
> > Thoughts?
> >
> 
> Copying here the answer to Parav, feel free to answer to any thread or
> highlight if I missed something :). Using the admin vq proposal terminology of
> "device group".
> 
> --
> This would stop a device of a device
> group, but not the whole virtqueue group. If the admin VQ is offered by the
> PF (since it's not exposed to the guest), it will continue accepting requests as
> normal. If it's exposed in the VF, I think the best bet is to shadow it, since
> guest and host requests could conflict.
> 

vhost-vdpa device is exposed for a VF through vp-vdpa driver to user land.
Now vp-vdpa driver will have to choose between using config register vs using AQ to suspend/resume the device.

Why not always begin with more superior interface of AQ that address multiple of these needs for LM case?

For LM case, more you explore, we realize that either VF relying on PF's AQ for query/config/setup/restore makes more sense or have its own dedicated AQ.

VM's suspend/resume operation can be handled through the shadow Q.

> Since this is offered through vdpa, the device backend driver can route it to
> whatever method works better for the hardware. For example, to send an
> admin vq command to the PF. That's why it's important to keep the feature
> as self-contained and orthogonal to others as possible.
> --
> 
> > > --
> > > 2.31.1
> >

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

RE: [PATCH v4 3/4] vhost-vdpa: uAPI to stop the device

[Parav Pandit]

> From: Eugenio Perez Martin <eperezma@redhat.com>
> Sent: Wednesday, June 1, 2022 7:15 AM
> 
> On Wed, Jun 1, 2022 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, May 26, 2022 at 02:43:37PM +0200, Eugenio Pérez wrote:
> > > The ioctl adds support for stop the device from userspace.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  drivers/vhost/vdpa.c       | 18 ++++++++++++++++++
> > >  include/uapi/linux/vhost.h | 14 ++++++++++++++
> > >  2 files changed, 32 insertions(+)
> > >
> > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index
> > > 32713db5831d..d1d19555c4b7 100644
> > > --- a/drivers/vhost/vdpa.c
> > > +++ b/drivers/vhost/vdpa.c
> > > @@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct
> vhost_vdpa *v, u32 __user *argp)
> > >       return 0;
> > >  }
> > >
> > > +static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
> > > +{
> > > +     struct vdpa_device *vdpa = v->vdpa;
> > > +     const struct vdpa_config_ops *ops = vdpa->config;
> > > +     int stop;
> > > +
> > > +     if (!ops->stop)
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     if (copy_from_user(&stop, argp, sizeof(stop)))
> > > +             return -EFAULT;
> > > +
> > > +     return ops->stop(vdpa, stop);
> > > +}
> > > +
> > >  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int
> cmd,
> > >                                  void __user *argp)  { @@ -650,6
> > > +665,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_STOP:
> > > +             r = vhost_vdpa_stop(v, argp);
> > > +             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..c7e47b29bf61 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)
> > >
> > > +/* Stop or resume a device so it does not process virtqueue
> > > +requests anymore
> > > + *
> > > + * After the return of ioctl with stop != 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 stop == 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_STOP                      _IOW(VHOST_VIRTIO, 0x7D, int)
> > > +
A better name is VHOST_VDPA_SET_STATE
State = stop/suspend
State = start/resume

Suspend/resume seems more logical, as opposed start/stop, because it more clearly indicates that the resume (start) is from some programmed beginning (and not first boot).

> > >  #endif
> >
> > I wonder how does this interact with the admin vq idea.
> > I.e. if we stop all VQs then apparently admin vq can't work either ...
> > Thoughts?
> >
> 
> Copying here the answer to Parav, feel free to answer to any thread or
> highlight if I missed something :). Using the admin vq proposal terminology of
> "device group".
> 
> --
> This would stop a device of a device
> group, but not the whole virtqueue group. If the admin VQ is offered by the
> PF (since it's not exposed to the guest), it will continue accepting requests as
> normal. If it's exposed in the VF, I think the best bet is to shadow it, since
> guest and host requests could conflict.
> 

vhost-vdpa device is exposed for a VF through vp-vdpa driver to user land.
Now vp-vdpa driver will have to choose between using config register vs using AQ to suspend/resume the device.

Why not always begin with more superior interface of AQ that address multiple of these needs for LM case?

For LM case, more you explore, we realize that either VF relying on PF's AQ for query/config/setup/restore makes more sense or have its own dedicated AQ.

VM's suspend/resume operation can be handled through the shadow Q.

> Since this is offered through vdpa, the device backend driver can route it to
> whatever method works better for the hardware. For example, to send an
> admin vq command to the PF. That's why it's important to keep the feature
> as self-contained and orthogonal to others as possible.
> --
> 
> > > --
> > > 2.31.1
> >

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit via Virtualization]

> From: Eugenio Perez Martin <eperezma@redhat.com>
> Sent: Wednesday, June 1, 2022 5:50 AM
> 
> On Tue, May 31, 2022 at 10:19 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Sunday, May 29, 2022 11:39 PM
> > >
> > > On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com>
> wrote:
> > > >
> > > > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > > > >
> > > > >
> > > > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > > > Sent: Thursday, May 26, 2022 8:44 AM
> > > > >
> > > > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa
> > > > > > will offer
> > > > > >
> > > > > > that backend feature and userspace can effectively stop 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).
> > > > > >
> > > > > >
> > > > > >
> > > > > > After the return of ioctl with stop != 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 stop == 0, the device can
> > > > > > continue
> > > > > >
> > > > > > processing buffers as long as typical conditions are met (vq
> > > > > > is enabled,
> > > > > >
> > > > > > DRIVER_OK status bit is enabled, etc).
> > > > >
> > > > > Just to be clear, we are adding vdpa level new ioctl() that
> > > > > doesn’t map to
> > > any mechanism in the virtio spec.
> > > > >
> > > > > Why can't we use this ioctl() to indicate driver to start/stop
> > > > > the device
> > > instead of driving it through the driver_ok?
> > > > > This is in the context of other discussion we had in the LM series.
> > > >
> > > > If there's something in the spec that does this then let's use that.
> > >
> > > Actually, we try to propose a independent feature here:
> > >
> > > https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.htm
> > > l
> > >
> > This will stop the device for all the operations.
> > Once the device is stopped, its state cannot be queried further as device
> won't respond.
> > It has limited use case.
> > What we need is to stop non admin queue related portion of the device.
> >
> 
> Still don't follow this, sorry.
Once a device it stopped its state etc cannot be queried.
if you want to stop and still allow certain operations, a better spec definition is needed that says,

stop A, B, C, but allow D, E, F, G.
A = stop CVQs and save its state somewhere
B = stop data VQs and save it state somewhere
C = stop generic config interrupt

D = query state of multiple VQs
E = query device statistics and other elements/objects in future
F = setup/config/restore certain fields
G = resume the device

> 
> Adding the admin vq to the mix, this would stop a device of a device group,
> but not the whole virtqueue group. If the admin VQ is offered by the PF
> (since it's not exposed to the guest), it will continue accepting requests as
> normal. If it's exposed in the VF, I think the best bet is to shadow it, since
> guest and host requests could conflict.
> 
> Since this is offered through vdpa, the device backend driver can route it to
> whatever method works better for the hardware. For example, to send an
> admin vq command to the PF. That's why it's important to keep the feature
> as self-contained and orthogonal to others as possible.
> 

I replied in other thread to continue there.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit]

> From: Eugenio Perez Martin <eperezma@redhat.com>
> Sent: Wednesday, June 1, 2022 5:50 AM
> 
> On Tue, May 31, 2022 at 10:19 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Sunday, May 29, 2022 11:39 PM
> > >
> > > On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com>
> wrote:
> > > >
> > > > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > > > >
> > > > >
> > > > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > > > Sent: Thursday, May 26, 2022 8:44 AM
> > > > >
> > > > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa
> > > > > > will offer
> > > > > >
> > > > > > that backend feature and userspace can effectively stop 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).
> > > > > >
> > > > > >
> > > > > >
> > > > > > After the return of ioctl with stop != 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 stop == 0, the device can
> > > > > > continue
> > > > > >
> > > > > > processing buffers as long as typical conditions are met (vq
> > > > > > is enabled,
> > > > > >
> > > > > > DRIVER_OK status bit is enabled, etc).
> > > > >
> > > > > Just to be clear, we are adding vdpa level new ioctl() that
> > > > > doesn’t map to
> > > any mechanism in the virtio spec.
> > > > >
> > > > > Why can't we use this ioctl() to indicate driver to start/stop
> > > > > the device
> > > instead of driving it through the driver_ok?
> > > > > This is in the context of other discussion we had in the LM series.
> > > >
> > > > If there's something in the spec that does this then let's use that.
> > >
> > > Actually, we try to propose a independent feature here:
> > >
> > > https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.htm
> > > l
> > >
> > This will stop the device for all the operations.
> > Once the device is stopped, its state cannot be queried further as device
> won't respond.
> > It has limited use case.
> > What we need is to stop non admin queue related portion of the device.
> >
> 
> Still don't follow this, sorry.
Once a device it stopped its state etc cannot be queried.
if you want to stop and still allow certain operations, a better spec definition is needed that says,

stop A, B, C, but allow D, E, F, G.
A = stop CVQs and save its state somewhere
B = stop data VQs and save it state somewhere
C = stop generic config interrupt

D = query state of multiple VQs
E = query device statistics and other elements/objects in future
F = setup/config/restore certain fields
G = resume the device

> 
> Adding the admin vq to the mix, this would stop a device of a device group,
> but not the whole virtqueue group. If the admin VQ is offered by the PF
> (since it's not exposed to the guest), it will continue accepting requests as
> normal. If it's exposed in the VF, I think the best bet is to shadow it, since
> guest and host requests could conflict.
> 
> Since this is offered through vdpa, the device backend driver can route it to
> whatever method works better for the hardware. For example, to send an
> admin vq command to the PF. That's why it's important to keep the feature
> as self-contained and orthogonal to others as possible.
> 

I replied in other thread to continue there.

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Thu, Jun 2, 2022 at 2:58 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, May 31, 2022 10:42 PM
> >
> > Well, the ability to query the virtqueue state was proposed as another
> > feature (Eugenio, please correct me). This should be sufficient for making
> > virtio-net to be live migrated.
> >
> The device is stopped, it won't answer to this special vq config done here.

This depends on the definition of the stop. Any query to the device
state should be allowed otherwise it's meaningless for us.

> Programming all of these using cfg registers doesn't scale for on-chip memory and for the speed.

Well, they are orthogonal and what I want to say is, we should first
define the semantics of stop and state of the virtqueue.

Such a facility could be accessed by either transport specific method
or admin virtqueue, it totally depends on the hardware architecture of
the vendor.

>
> Next would be to program hundreds of statistics of the 64 VQs through a giant PCI config space register in some busy polling scheme.

We don't need giant config space, and this method has been implemented
by some vDPA vendors.

>
> I can clearly see how all these are inefficient for faster LM.
> We need an efficient AQ to proceed with at minimum.

I'm fine with admin virtqueue, but the stop and state are orthogonal
to that. And using admin virtqueue for stop/state will be more natural
if we use admin virtqueue as a transport.

Thanks

>
> > https://lists.oasis-open.org/archives/virtio-comment/202103/msg00008.html
> >
> > > Once the device is stopped, its state cannot be queried further as device
> > won't respond.
> > > It has limited use case.
> > > What we need is to stop non admin queue related portion of the device.

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Thu, Jun 2, 2022 at 2:58 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, May 31, 2022 10:42 PM
> >
> > Well, the ability to query the virtqueue state was proposed as another
> > feature (Eugenio, please correct me). This should be sufficient for making
> > virtio-net to be live migrated.
> >
> The device is stopped, it won't answer to this special vq config done here.

This depends on the definition of the stop. Any query to the device
state should be allowed otherwise it's meaningless for us.

> Programming all of these using cfg registers doesn't scale for on-chip memory and for the speed.

Well, they are orthogonal and what I want to say is, we should first
define the semantics of stop and state of the virtqueue.

Such a facility could be accessed by either transport specific method
or admin virtqueue, it totally depends on the hardware architecture of
the vendor.

>
> Next would be to program hundreds of statistics of the 64 VQs through a giant PCI config space register in some busy polling scheme.

We don't need giant config space, and this method has been implemented
by some vDPA vendors.

>
> I can clearly see how all these are inefficient for faster LM.
> We need an efficient AQ to proceed with at minimum.

I'm fine with admin virtqueue, but the stop and state are orthogonal
to that. And using admin virtqueue for stop/state will be more natural
if we use admin virtqueue as a transport.

Thanks

>
> > https://lists.oasis-open.org/archives/virtio-comment/202103/msg00008.html
> >
> > > Once the device is stopped, its state cannot be queried further as device
> > won't respond.
> > > It has limited use case.
> > > What we need is to stop non admin queue related portion of the device.

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

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Thu, Jun 2, 2022 at 3:30 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Eugenio Perez Martin <eperezma@redhat.com>
> > Sent: Wednesday, June 1, 2022 5:50 AM
> >
> > On Tue, May 31, 2022 at 10:19 PM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Sunday, May 29, 2022 11:39 PM
> > > >
> > > > On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com>
> > wrote:
> > > > >
> > > > > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > > > > >
> > > > > >
> > > > > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > Sent: Thursday, May 26, 2022 8:44 AM
> > > > > >
> > > > > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa
> > > > > > > will offer
> > > > > > >
> > > > > > > that backend feature and userspace can effectively stop 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).
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > After the return of ioctl with stop != 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 stop == 0, the device can
> > > > > > > continue
> > > > > > >
> > > > > > > processing buffers as long as typical conditions are met (vq
> > > > > > > is enabled,
> > > > > > >
> > > > > > > DRIVER_OK status bit is enabled, etc).
> > > > > >
> > > > > > Just to be clear, we are adding vdpa level new ioctl() that
> > > > > > doesn’t map to
> > > > any mechanism in the virtio spec.
> > > > > >
> > > > > > Why can't we use this ioctl() to indicate driver to start/stop
> > > > > > the device
> > > > instead of driving it through the driver_ok?
> > > > > > This is in the context of other discussion we had in the LM series.
> > > > >
> > > > > If there's something in the spec that does this then let's use that.
> > > >
> > > > Actually, we try to propose a independent feature here:
> > > >
> > > > https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.htm
> > > > l
> > > >
> > > This will stop the device for all the operations.
> > > Once the device is stopped, its state cannot be queried further as device
> > won't respond.
> > > It has limited use case.
> > > What we need is to stop non admin queue related portion of the device.
> > >
> >
> > Still don't follow this, sorry.
> Once a device it stopped its state etc cannot be queried.

This is not what is proposed here.

> if you want to stop and still allow certain operations, a better spec definition is needed that says,
>
> stop A, B, C, but allow D, E, F, G.
> A = stop CVQs and save its state somewhere
> B = stop data VQs and save it state somewhere
> C = stop generic config interrupt

Actually, it's the stop of the config space change.
And what more, any guest visible state must not be changed.

>
> D = query state of multiple VQs
> E = query device statistics and other elements/objects in future

This is the device state I believe.

> F = setup/config/restore certain fields

This is the reverse of D and E, that is setting the state.

> G = resume the device
>

Thanks

> >
> > Adding the admin vq to the mix, this would stop a device of a device group,
> > but not the whole virtqueue group. If the admin VQ is offered by the PF
> > (since it's not exposed to the guest), it will continue accepting requests as
> > normal. If it's exposed in the VF, I think the best bet is to shadow it, since
> > guest and host requests could conflict.
> >
> > Since this is offered through vdpa, the device backend driver can route it to
> > whatever method works better for the hardware. For example, to send an
> > admin vq command to the PF. That's why it's important to keep the feature
> > as self-contained and orthogonal to others as possible.
> >
>
> I replied in other thread to continue there.

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Thu, Jun 2, 2022 at 3:30 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Eugenio Perez Martin <eperezma@redhat.com>
> > Sent: Wednesday, June 1, 2022 5:50 AM
> >
> > On Tue, May 31, 2022 at 10:19 PM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Sunday, May 29, 2022 11:39 PM
> > > >
> > > > On Fri, May 27, 2022 at 6:56 PM Michael S. Tsirkin <mst@redhat.com>
> > wrote:
> > > > >
> > > > > On Thu, May 26, 2022 at 12:54:32PM +0000, Parav Pandit wrote:
> > > > > >
> > > > > >
> > > > > > > From: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > Sent: Thursday, May 26, 2022 8:44 AM
> > > > > >
> > > > > > > Implement stop operation for vdpa_sim devices, so vhost-vdpa
> > > > > > > will offer
> > > > > > >
> > > > > > > that backend feature and userspace can effectively stop 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).
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > After the return of ioctl with stop != 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 stop == 0, the device can
> > > > > > > continue
> > > > > > >
> > > > > > > processing buffers as long as typical conditions are met (vq
> > > > > > > is enabled,
> > > > > > >
> > > > > > > DRIVER_OK status bit is enabled, etc).
> > > > > >
> > > > > > Just to be clear, we are adding vdpa level new ioctl() that
> > > > > > doesn’t map to
> > > > any mechanism in the virtio spec.
> > > > > >
> > > > > > Why can't we use this ioctl() to indicate driver to start/stop
> > > > > > the device
> > > > instead of driving it through the driver_ok?
> > > > > > This is in the context of other discussion we had in the LM series.
> > > > >
> > > > > If there's something in the spec that does this then let's use that.
> > > >
> > > > Actually, we try to propose a independent feature here:
> > > >
> > > > https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.htm
> > > > l
> > > >
> > > This will stop the device for all the operations.
> > > Once the device is stopped, its state cannot be queried further as device
> > won't respond.
> > > It has limited use case.
> > > What we need is to stop non admin queue related portion of the device.
> > >
> >
> > Still don't follow this, sorry.
> Once a device it stopped its state etc cannot be queried.

This is not what is proposed here.

> if you want to stop and still allow certain operations, a better spec definition is needed that says,
>
> stop A, B, C, but allow D, E, F, G.
> A = stop CVQs and save its state somewhere
> B = stop data VQs and save it state somewhere
> C = stop generic config interrupt

Actually, it's the stop of the config space change.
And what more, any guest visible state must not be changed.

>
> D = query state of multiple VQs
> E = query device statistics and other elements/objects in future

This is the device state I believe.

> F = setup/config/restore certain fields

This is the reverse of D and E, that is setting the state.

> G = resume the device
>

Thanks

> >
> > Adding the admin vq to the mix, this would stop a device of a device group,
> > but not the whole virtqueue group. If the admin VQ is offered by the PF
> > (since it's not exposed to the guest), it will continue accepting requests as
> > normal. If it's exposed in the VF, I think the best bet is to shadow it, since
> > guest and host requests could conflict.
> >
> > Since this is offered through vdpa, the device backend driver can route it to
> > whatever method works better for the hardware. For example, to send an
> > admin vq command to the PF. That's why it's important to keep the feature
> > as self-contained and orthogonal to others as possible.
> >
>
> I replied in other thread to continue there.

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

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

在 2022/5/26 20:43, Eugenio Pérez 写道:
> Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> that backend feature and userspace can effectively stop 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).
>
> After the return of ioctl with stop != 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.


I think we probably need more accurate definition on the state as Parav 
suggested.

Besides this, we should also clarify when stop is allowed. E.g should we 
allow setting stop without DRIVER_OK?

Thanks


>
> After the return of ioctl with stop == 0, the device can continue
> processing buffers as long as typical conditions are met (vq is enabled,
> DRIVER_OK status bit is enabled, etc).
>
> In the future, we will provide features similar to VHOST_USER_GET_INFLIGHT_FD
> so the device can save pending operations.
>
> Comments are welcome.
>
> 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 stop operation
>    vhost-vdpa: introduce STOP backend feature bit
>    vhost-vdpa: uAPI to stop the device
>    vdpa_sim: Implement stop 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                 | 34 +++++++++++++++++++++++++++-
>   include/linux/vdpa.h                 |  6 +++++
>   include/uapi/linux/vhost.h           | 14 ++++++++++++
>   include/uapi/linux/vhost_types.h     |  2 ++
>   8 files changed, 83 insertions(+), 1 deletion(-)
>

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

在 2022/5/26 20:43, Eugenio Pérez 写道:
> Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
> that backend feature and userspace can effectively stop 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).
>
> After the return of ioctl with stop != 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.


I think we probably need more accurate definition on the state as Parav 
suggested.

Besides this, we should also clarify when stop is allowed. E.g should we 
allow setting stop without DRIVER_OK?

Thanks


>
> After the return of ioctl with stop == 0, the device can continue
> processing buffers as long as typical conditions are met (vq is enabled,
> DRIVER_OK status bit is enabled, etc).
>
> In the future, we will provide features similar to VHOST_USER_GET_INFLIGHT_FD
> so the device can save pending operations.
>
> Comments are welcome.
>
> 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 stop operation
>    vhost-vdpa: introduce STOP backend feature bit
>    vhost-vdpa: uAPI to stop the device
>    vdpa_sim: Implement stop 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                 | 34 +++++++++++++++++++++++++++-
>   include/linux/vdpa.h                 |  6 +++++
>   include/uapi/linux/vhost.h           | 14 ++++++++++++
>   include/uapi/linux/vhost_types.h     |  2 ++
>   8 files changed, 83 insertions(+), 1 deletion(-)
>

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

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, June 1, 2022 10:00 PM
> 
> On Thu, Jun 2, 2022 at 2:58 AM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Tuesday, May 31, 2022 10:42 PM
> > >
> > > Well, the ability to query the virtqueue state was proposed as
> > > another feature (Eugenio, please correct me). This should be
> > > sufficient for making virtio-net to be live migrated.
> > >
> > The device is stopped, it won't answer to this special vq config done here.
> 
> This depends on the definition of the stop. Any query to the device state
> should be allowed otherwise it's meaningless for us.
> 
> > Programming all of these using cfg registers doesn't scale for on-chip
> memory and for the speed.
> 
> Well, they are orthogonal and what I want to say is, we should first define
> the semantics of stop and state of the virtqueue.
> 
> Such a facility could be accessed by either transport specific method or admin
> virtqueue, it totally depends on the hardware architecture of the vendor.
> 
I find it hard to believe that a vendor can implement a CVQ but not AQ and chose to expose tens of hundreds of registers.
But maybe, it fits some specific hw.

I like to learn the advantages of such method other than simplicity.

We can clearly that we are shifting away from such PCI registers with SIOV, IMS and other scalable solutions.
virtio drifting in reverse direction by introducing more registers as transport.
I expect it to an optional transport like AQ.

> >
> > Next would be to program hundreds of statistics of the 64 VQs through a
> giant PCI config space register in some busy polling scheme.
> 
> We don't need giant config space, and this method has been implemented
> by some vDPA vendors.
> 
There are tens of 64-bit counters per VQs. These needs to programmed on destination side.
Programming these via registers requires exposing them on the registers.
In one of the proposals, I see them being queried via CVQ from the device.

Programming them via cfg registers requires large cfg space or synchronous programming until receiving ACK from it.
This means one entry at a time...

Programming them via CVQ needs replicate and align cmd values etc on all device types. All duplicate and hard to maintain.


> >
> > I can clearly see how all these are inefficient for faster LM.
> > We need an efficient AQ to proceed with at minimum.
> 
> I'm fine with admin virtqueue, but the stop and state are orthogonal to that.
> And using admin virtqueue for stop/state will be more natural if we use
> admin virtqueue as a transport.
Ok.
We should have defined it bit earlier that all vendors can use. :(

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit via Virtualization]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, June 1, 2022 10:00 PM
> 
> On Thu, Jun 2, 2022 at 2:58 AM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Tuesday, May 31, 2022 10:42 PM
> > >
> > > Well, the ability to query the virtqueue state was proposed as
> > > another feature (Eugenio, please correct me). This should be
> > > sufficient for making virtio-net to be live migrated.
> > >
> > The device is stopped, it won't answer to this special vq config done here.
> 
> This depends on the definition of the stop. Any query to the device state
> should be allowed otherwise it's meaningless for us.
> 
> > Programming all of these using cfg registers doesn't scale for on-chip
> memory and for the speed.
> 
> Well, they are orthogonal and what I want to say is, we should first define
> the semantics of stop and state of the virtqueue.
> 
> Such a facility could be accessed by either transport specific method or admin
> virtqueue, it totally depends on the hardware architecture of the vendor.
> 
I find it hard to believe that a vendor can implement a CVQ but not AQ and chose to expose tens of hundreds of registers.
But maybe, it fits some specific hw.

I like to learn the advantages of such method other than simplicity.

We can clearly that we are shifting away from such PCI registers with SIOV, IMS and other scalable solutions.
virtio drifting in reverse direction by introducing more registers as transport.
I expect it to an optional transport like AQ.

> >
> > Next would be to program hundreds of statistics of the 64 VQs through a
> giant PCI config space register in some busy polling scheme.
> 
> We don't need giant config space, and this method has been implemented
> by some vDPA vendors.
> 
There are tens of 64-bit counters per VQs. These needs to programmed on destination side.
Programming these via registers requires exposing them on the registers.
In one of the proposals, I see them being queried via CVQ from the device.

Programming them via cfg registers requires large cfg space or synchronous programming until receiving ACK from it.
This means one entry at a time...

Programming them via CVQ needs replicate and align cmd values etc on all device types. All duplicate and hard to maintain.


> >
> > I can clearly see how all these are inefficient for faster LM.
> > We need an efficient AQ to proceed with at minimum.
> 
> I'm fine with admin virtqueue, but the stop and state are orthogonal to that.
> And using admin virtqueue for stop/state will be more natural if we use
> admin virtqueue as a transport.
Ok.
We should have defined it bit earlier that all vendors can use. :(
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Thu, Jun 2, 2022 at 10:59 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, June 1, 2022 10:00 PM
> >
> > On Thu, Jun 2, 2022 at 2:58 AM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Tuesday, May 31, 2022 10:42 PM
> > > >
> > > > Well, the ability to query the virtqueue state was proposed as
> > > > another feature (Eugenio, please correct me). This should be
> > > > sufficient for making virtio-net to be live migrated.
> > > >
> > > The device is stopped, it won't answer to this special vq config done here.
> >
> > This depends on the definition of the stop. Any query to the device state
> > should be allowed otherwise it's meaningless for us.
> >
> > > Programming all of these using cfg registers doesn't scale for on-chip
> > memory and for the speed.
> >
> > Well, they are orthogonal and what I want to say is, we should first define
> > the semantics of stop and state of the virtqueue.
> >
> > Such a facility could be accessed by either transport specific method or admin
> > virtqueue, it totally depends on the hardware architecture of the vendor.
> >
> I find it hard to believe that a vendor can implement a CVQ but not AQ and chose to expose tens of hundreds of registers.
> But maybe, it fits some specific hw.

You can have a look at the ifcvf dpdk driver as an example.

But another thing that is unrelated to hardware architecture is the
nesting support. Having admin virtqueue in a nesting environment looks
like an overkill. Presenting a register in L1 and map it to L0's admin
should be good enough.

>
> I like to learn the advantages of such method other than simplicity.
>
> We can clearly that we are shifting away from such PCI registers with SIOV, IMS and other scalable solutions.
> virtio drifting in reverse direction by introducing more registers as transport.
> I expect it to an optional transport like AQ.

Actually, I had a proposal of using admin virtqueue as a transport,
it's designed to be SIOV/IMS capable. And it's not hard to extend it
with the state/stop support etc.

>
> > >
> > > Next would be to program hundreds of statistics of the 64 VQs through a
> > giant PCI config space register in some busy polling scheme.
> >
> > We don't need giant config space, and this method has been implemented
> > by some vDPA vendors.
> >
> There are tens of 64-bit counters per VQs. These needs to programmed on destination side.
> Programming these via registers requires exposing them on the registers.
> In one of the proposals, I see them being queried via CVQ from the device.

I didn't see a proposal like this. And I don't think querying general
virtio state like idx with a device specific CVQ is a good design.

>
> Programming them via cfg registers requires large cfg space or synchronous programming until receiving ACK from it.
> This means one entry at a time...
>
> Programming them via CVQ needs replicate and align cmd values etc on all device types. All duplicate and hard to maintain.
>
>
> > >
> > > I can clearly see how all these are inefficient for faster LM.
> > > We need an efficient AQ to proceed with at minimum.
> >
> > I'm fine with admin virtqueue, but the stop and state are orthogonal to that.
> > And using admin virtqueue for stop/state will be more natural if we use
> > admin virtqueue as a transport.
> Ok.
> We should have defined it bit earlier that all vendors can use. :(

I agree.

Thanks

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Thu, Jun 2, 2022 at 10:59 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, June 1, 2022 10:00 PM
> >
> > On Thu, Jun 2, 2022 at 2:58 AM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Tuesday, May 31, 2022 10:42 PM
> > > >
> > > > Well, the ability to query the virtqueue state was proposed as
> > > > another feature (Eugenio, please correct me). This should be
> > > > sufficient for making virtio-net to be live migrated.
> > > >
> > > The device is stopped, it won't answer to this special vq config done here.
> >
> > This depends on the definition of the stop. Any query to the device state
> > should be allowed otherwise it's meaningless for us.
> >
> > > Programming all of these using cfg registers doesn't scale for on-chip
> > memory and for the speed.
> >
> > Well, they are orthogonal and what I want to say is, we should first define
> > the semantics of stop and state of the virtqueue.
> >
> > Such a facility could be accessed by either transport specific method or admin
> > virtqueue, it totally depends on the hardware architecture of the vendor.
> >
> I find it hard to believe that a vendor can implement a CVQ but not AQ and chose to expose tens of hundreds of registers.
> But maybe, it fits some specific hw.

You can have a look at the ifcvf dpdk driver as an example.

But another thing that is unrelated to hardware architecture is the
nesting support. Having admin virtqueue in a nesting environment looks
like an overkill. Presenting a register in L1 and map it to L0's admin
should be good enough.

>
> I like to learn the advantages of such method other than simplicity.
>
> We can clearly that we are shifting away from such PCI registers with SIOV, IMS and other scalable solutions.
> virtio drifting in reverse direction by introducing more registers as transport.
> I expect it to an optional transport like AQ.

Actually, I had a proposal of using admin virtqueue as a transport,
it's designed to be SIOV/IMS capable. And it's not hard to extend it
with the state/stop support etc.

>
> > >
> > > Next would be to program hundreds of statistics of the 64 VQs through a
> > giant PCI config space register in some busy polling scheme.
> >
> > We don't need giant config space, and this method has been implemented
> > by some vDPA vendors.
> >
> There are tens of 64-bit counters per VQs. These needs to programmed on destination side.
> Programming these via registers requires exposing them on the registers.
> In one of the proposals, I see them being queried via CVQ from the device.

I didn't see a proposal like this. And I don't think querying general
virtio state like idx with a device specific CVQ is a good design.

>
> Programming them via cfg registers requires large cfg space or synchronous programming until receiving ACK from it.
> This means one entry at a time...
>
> Programming them via CVQ needs replicate and align cmd values etc on all device types. All duplicate and hard to maintain.
>
>
> > >
> > > I can clearly see how all these are inefficient for faster LM.
> > > We need an efficient AQ to proceed with at minimum.
> >
> > I'm fine with admin virtqueue, but the stop and state are orthogonal to that.
> > And using admin virtqueue for stop/state will be more natural if we use
> > admin virtqueue as a transport.
> Ok.
> We should have defined it bit earlier that all vendors can use. :(

I agree.

Thanks

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

Re: [PATCH v4 3/4] vhost-vdpa: uAPI to stop the device

[Eugenio Perez Martin]

On Wed, Jun 1, 2022 at 9:13 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Eugenio Perez Martin <eperezma@redhat.com>
> > Sent: Wednesday, June 1, 2022 7:15 AM
> >
> > On Wed, Jun 1, 2022 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, May 26, 2022 at 02:43:37PM +0200, Eugenio Pérez wrote:
> > > > The ioctl adds support for stop the device from userspace.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >  drivers/vhost/vdpa.c       | 18 ++++++++++++++++++
> > > >  include/uapi/linux/vhost.h | 14 ++++++++++++++
> > > >  2 files changed, 32 insertions(+)
> > > >
> > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index
> > > > 32713db5831d..d1d19555c4b7 100644
> > > > --- a/drivers/vhost/vdpa.c
> > > > +++ b/drivers/vhost/vdpa.c
> > > > @@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct
> > vhost_vdpa *v, u32 __user *argp)
> > > >       return 0;
> > > >  }
> > > >
> > > > +static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
> > > > +{
> > > > +     struct vdpa_device *vdpa = v->vdpa;
> > > > +     const struct vdpa_config_ops *ops = vdpa->config;
> > > > +     int stop;
> > > > +
> > > > +     if (!ops->stop)
> > > > +             return -EOPNOTSUPP;
> > > > +
> > > > +     if (copy_from_user(&stop, argp, sizeof(stop)))
> > > > +             return -EFAULT;
> > > > +
> > > > +     return ops->stop(vdpa, stop);
> > > > +}
> > > > +
> > > >  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int
> > cmd,
> > > >                                  void __user *argp)  { @@ -650,6
> > > > +665,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_STOP:
> > > > +             r = vhost_vdpa_stop(v, argp);
> > > > +             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..c7e47b29bf61 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)
> > > >
> > > > +/* Stop or resume a device so it does not process virtqueue
> > > > +requests anymore
> > > > + *
> > > > + * After the return of ioctl with stop != 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 stop == 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_STOP                      _IOW(VHOST_VIRTIO, 0x7D, int)
> > > > +
> A better name is VHOST_VDPA_SET_STATE
> State = stop/suspend
> State = start/resume
>
> Suspend/resume seems more logical, as opposed start/stop, because it more clearly indicates that the resume (start) is from some programmed beginning (and not first boot).
>

It's fine to move to that nomenclature in my opinion.

> > > >  #endif
> > >
> > > I wonder how does this interact with the admin vq idea.
> > > I.e. if we stop all VQs then apparently admin vq can't work either ...
> > > Thoughts?
> > >
> >
> > Copying here the answer to Parav, feel free to answer to any thread or
> > highlight if I missed something :). Using the admin vq proposal terminology of
> > "device group".
> >
> > --
> > This would stop a device of a device
> > group, but not the whole virtqueue group. If the admin VQ is offered by the
> > PF (since it's not exposed to the guest), it will continue accepting requests as
> > normal. If it's exposed in the VF, I think the best bet is to shadow it, since
> > guest and host requests could conflict.
> >
>
> vhost-vdpa device is exposed for a VF through vp-vdpa driver to user land.
> Now vp-vdpa driver will have to choose between using config register vs using AQ to suspend/resume the device.
>

vp_vdpa cannot choose if the virtio device has an admin vq or any
other feature, it just wraps the virtio device. If that virtio device
does not expose AQ, vp_vdpa cannot expose it.

> Why not always begin with more superior interface of AQ that address multiple of these needs for LM case?
>

Because it doesn't address valid use cases like vp_vdpa with no AQ,
devices that are not VF, or nested virtualization.

VHOST_VDPA_STOP / VHOST_VDPA_SET_STATE does not replace AQ commands:
It's just the way vhost-vdpa exposes that capability to qemu. vdpa
backend is free to choose whatever methods it finds better to
implement it.

> For LM case, more you explore, we realize that either VF relying on PF's AQ for query/config/setup/restore makes more sense or have its own dedicated AQ.
>

This ioctl does not mandate that the device cannot implement it
through AQ, or that the device has to be a VF.

Thanks!

> VM's suspend/resume operation can be handled through the shadow Q.
>
> > Since this is offered through vdpa, the device backend driver can route it to
> > whatever method works better for the hardware. For example, to send an
> > admin vq command to the PF. That's why it's important to keep the feature
> > as self-contained and orthogonal to others as possible.
> > --
> >
> > > > --
> > > > 2.31.1
> > >
>

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Eugenio Perez Martin]

On Thu, Jun 2, 2022 at 4:59 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, June 1, 2022 10:00 PM
> >
> > On Thu, Jun 2, 2022 at 2:58 AM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Tuesday, May 31, 2022 10:42 PM
> > > >
> > > > Well, the ability to query the virtqueue state was proposed as
> > > > another feature (Eugenio, please correct me). This should be
> > > > sufficient for making virtio-net to be live migrated.
> > > >
> > > The device is stopped, it won't answer to this special vq config done here.
> >
> > This depends on the definition of the stop. Any query to the device state
> > should be allowed otherwise it's meaningless for us.
> >
> > > Programming all of these using cfg registers doesn't scale for on-chip
> > memory and for the speed.
> >
> > Well, they are orthogonal and what I want to say is, we should first define
> > the semantics of stop and state of the virtqueue.
> >
> > Such a facility could be accessed by either transport specific method or admin
> > virtqueue, it totally depends on the hardware architecture of the vendor.
> >
> I find it hard to believe that a vendor can implement a CVQ but not AQ and chose to expose tens of hundreds of registers.
> But maybe, it fits some specific hw.
>
> I like to learn the advantages of such method other than simplicity.
>
> We can clearly that we are shifting away from such PCI registers with SIOV, IMS and other scalable solutions.
> virtio drifting in reverse direction by introducing more registers as transport.
> I expect it to an optional transport like AQ.
>
> > >
> > > Next would be to program hundreds of statistics of the 64 VQs through a
> > giant PCI config space register in some busy polling scheme.
> >
> > We don't need giant config space, and this method has been implemented
> > by some vDPA vendors.
> >
> There are tens of 64-bit counters per VQs. These needs to programmed on destination side.
> Programming these via registers requires exposing them on the registers.
> In one of the proposals, I see them being queried via CVQ from the device.
>
> Programming them via cfg registers requires large cfg space or synchronous programming until receiving ACK from it.
> This means one entry at a time...
>
> Programming them via CVQ needs replicate and align cmd values etc on all device types. All duplicate and hard to maintain.
>

I think this discussion should be moved to the proposals on
virtio-comment. In the vdpa context, they should be covered.

This one is about exposing the basic facility of stopping and resuming
a device to userland, and it fits equally well if the device
implements it via cfg registers, via admin vq, via channel I/O, or via
whatever transport the vdpa backend prefers. To ask for the state is
already covered in the vhost layer, and this proposal does not affect
it.

Given the flexibility of vdpa, we can even ask vq state using
backend-specific methods, cache it (knowing that there will be no
change of them until resume or DRIVER_OK), and expose them to qemu
using config space interface or any other batch method. Same as with
the enable_vq problem. And the same applies to stats. And we maintain
compatibility with all vendor-specific control plane.

Would that work for devices that cannot or does not want to expose
them via config space?

Thanks!

>
> > >
> > > I can clearly see how all these are inefficient for faster LM.
> > > We need an efficient AQ to proceed with at minimum.
> >
> > I'm fine with admin virtqueue, but the stop and state are orthogonal to that.
> > And using admin virtqueue for stop/state will be more natural if we use
> > admin virtqueue as a transport.
> Ok.
> We should have defined it bit earlier that all vendors can use. :(

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, June 1, 2022 11:54 PM
> 
> On Thu, Jun 2, 2022 at 10:59 AM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Wednesday, June 1, 2022 10:00 PM
> > >
> > > On Thu, Jun 2, 2022 at 2:58 AM Parav Pandit <parav@nvidia.com> wrote:
> > > >
> > > >
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Tuesday, May 31, 2022 10:42 PM
> > > > >
> > > > > Well, the ability to query the virtqueue state was proposed as
> > > > > another feature (Eugenio, please correct me). This should be
> > > > > sufficient for making virtio-net to be live migrated.
> > > > >
> > > > The device is stopped, it won't answer to this special vq config done
> here.
> > >
> > > This depends on the definition of the stop. Any query to the device
> > > state should be allowed otherwise it's meaningless for us.
> > >
> > > > Programming all of these using cfg registers doesn't scale for
> > > > on-chip
> > > memory and for the speed.
> > >
> > > Well, they are orthogonal and what I want to say is, we should first
> > > define the semantics of stop and state of the virtqueue.
> > >
> > > Such a facility could be accessed by either transport specific
> > > method or admin virtqueue, it totally depends on the hardware
> architecture of the vendor.
> > >
> > I find it hard to believe that a vendor can implement a CVQ but not AQ and
> chose to expose tens of hundreds of registers.
> > But maybe, it fits some specific hw.
> 
> You can have a look at the ifcvf dpdk driver as an example.
> 
Ifcvf is an example of using registers.
It is not an answer why AQ is hard for it. :)
virtio spec has definition of queue now and implementing yet another queue shouldn't be a problem.

So far no one seem to have problem with the additional queue.
So I take it as AQ is ok.

> But another thing that is unrelated to hardware architecture is the nesting
> support. Having admin virtqueue in a nesting environment looks like an
> overkill. Presenting a register in L1 and map it to L0's admin should be good
> enough.
So may be a optimized interface can be added that fits nested env.
At this point in time real users that we heard are interested in non-nested use cases. Let's enable them first.


> 
> >
> > I like to learn the advantages of such method other than simplicity.
> >
> > We can clearly that we are shifting away from such PCI registers with SIOV,
> IMS and other scalable solutions.
> > virtio drifting in reverse direction by introducing more registers as
> transport.
> > I expect it to an optional transport like AQ.
> 
> Actually, I had a proposal of using admin virtqueue as a transport, it's
> designed to be SIOV/IMS capable. And it's not hard to extend it with the
> state/stop support etc.
> 
> >
> > > >
> > > > Next would be to program hundreds of statistics of the 64 VQs
> > > > through a
> > > giant PCI config space register in some busy polling scheme.
> > >
> > > We don't need giant config space, and this method has been
> > > implemented by some vDPA vendors.
> > >
> > There are tens of 64-bit counters per VQs. These needs to programmed on
> destination side.
> > Programming these via registers requires exposing them on the registers.
> > In one of the proposals, I see them being queried via CVQ from the device.
> 
> I didn't see a proposal like this. And I don't think querying general virtio state
> like idx with a device specific CVQ is a good design.
> 
My example was not for the idx. But for VQ statistics that is queried via CVQ.

> >
> > Programming them via cfg registers requires large cfg space or synchronous
> programming until receiving ACK from it.
> > This means one entry at a time...
> >
> > Programming them via CVQ needs replicate and align cmd values etc on all
> device types. All duplicate and hard to maintain.
> >
> >
> > > >
> > > > I can clearly see how all these are inefficient for faster LM.
> > > > We need an efficient AQ to proceed with at minimum.
> > >
> > > I'm fine with admin virtqueue, but the stop and state are orthogonal to
> that.
> > > And using admin virtqueue for stop/state will be more natural if we
> > > use admin virtqueue as a transport.
> > Ok.
> > We should have defined it bit earlier that all vendors can use. :(
> 
> I agree.

I remember few months back, you acked in the weekly meeting that TC has approved the AQ direction.
And we are still in this circle of debating the AQ.

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit via Virtualization]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, June 1, 2022 11:54 PM
> 
> On Thu, Jun 2, 2022 at 10:59 AM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Wednesday, June 1, 2022 10:00 PM
> > >
> > > On Thu, Jun 2, 2022 at 2:58 AM Parav Pandit <parav@nvidia.com> wrote:
> > > >
> > > >
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Tuesday, May 31, 2022 10:42 PM
> > > > >
> > > > > Well, the ability to query the virtqueue state was proposed as
> > > > > another feature (Eugenio, please correct me). This should be
> > > > > sufficient for making virtio-net to be live migrated.
> > > > >
> > > > The device is stopped, it won't answer to this special vq config done
> here.
> > >
> > > This depends on the definition of the stop. Any query to the device
> > > state should be allowed otherwise it's meaningless for us.
> > >
> > > > Programming all of these using cfg registers doesn't scale for
> > > > on-chip
> > > memory and for the speed.
> > >
> > > Well, they are orthogonal and what I want to say is, we should first
> > > define the semantics of stop and state of the virtqueue.
> > >
> > > Such a facility could be accessed by either transport specific
> > > method or admin virtqueue, it totally depends on the hardware
> architecture of the vendor.
> > >
> > I find it hard to believe that a vendor can implement a CVQ but not AQ and
> chose to expose tens of hundreds of registers.
> > But maybe, it fits some specific hw.
> 
> You can have a look at the ifcvf dpdk driver as an example.
> 
Ifcvf is an example of using registers.
It is not an answer why AQ is hard for it. :)
virtio spec has definition of queue now and implementing yet another queue shouldn't be a problem.

So far no one seem to have problem with the additional queue.
So I take it as AQ is ok.

> But another thing that is unrelated to hardware architecture is the nesting
> support. Having admin virtqueue in a nesting environment looks like an
> overkill. Presenting a register in L1 and map it to L0's admin should be good
> enough.
So may be a optimized interface can be added that fits nested env.
At this point in time real users that we heard are interested in non-nested use cases. Let's enable them first.


> 
> >
> > I like to learn the advantages of such method other than simplicity.
> >
> > We can clearly that we are shifting away from such PCI registers with SIOV,
> IMS and other scalable solutions.
> > virtio drifting in reverse direction by introducing more registers as
> transport.
> > I expect it to an optional transport like AQ.
> 
> Actually, I had a proposal of using admin virtqueue as a transport, it's
> designed to be SIOV/IMS capable. And it's not hard to extend it with the
> state/stop support etc.
> 
> >
> > > >
> > > > Next would be to program hundreds of statistics of the 64 VQs
> > > > through a
> > > giant PCI config space register in some busy polling scheme.
> > >
> > > We don't need giant config space, and this method has been
> > > implemented by some vDPA vendors.
> > >
> > There are tens of 64-bit counters per VQs. These needs to programmed on
> destination side.
> > Programming these via registers requires exposing them on the registers.
> > In one of the proposals, I see them being queried via CVQ from the device.
> 
> I didn't see a proposal like this. And I don't think querying general virtio state
> like idx with a device specific CVQ is a good design.
> 
My example was not for the idx. But for VQ statistics that is queried via CVQ.

> >
> > Programming them via cfg registers requires large cfg space or synchronous
> programming until receiving ACK from it.
> > This means one entry at a time...
> >
> > Programming them via CVQ needs replicate and align cmd values etc on all
> device types. All duplicate and hard to maintain.
> >
> >
> > > >
> > > > I can clearly see how all these are inefficient for faster LM.
> > > > We need an efficient AQ to proceed with at minimum.
> > >
> > > I'm fine with admin virtqueue, but the stop and state are orthogonal to
> that.
> > > And using admin virtqueue for stop/state will be more natural if we
> > > use admin virtqueue as a transport.
> > Ok.
> > We should have defined it bit earlier that all vendors can use. :(
> 
> I agree.

I remember few months back, you acked in the weekly meeting that TC has approved the AQ direction.
And we are still in this circle of debating the AQ.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Wed, Jun 15, 2022 at 8:10 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, June 1, 2022 11:54 PM
> >
> > On Thu, Jun 2, 2022 at 10:59 AM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Wednesday, June 1, 2022 10:00 PM
> > > >
> > > > On Thu, Jun 2, 2022 at 2:58 AM Parav Pandit <parav@nvidia.com> wrote:
> > > > >
> > > > >
> > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > Sent: Tuesday, May 31, 2022 10:42 PM
> > > > > >
> > > > > > Well, the ability to query the virtqueue state was proposed as
> > > > > > another feature (Eugenio, please correct me). This should be
> > > > > > sufficient for making virtio-net to be live migrated.
> > > > > >
> > > > > The device is stopped, it won't answer to this special vq config done
> > here.
> > > >
> > > > This depends on the definition of the stop. Any query to the device
> > > > state should be allowed otherwise it's meaningless for us.
> > > >
> > > > > Programming all of these using cfg registers doesn't scale for
> > > > > on-chip
> > > > memory and for the speed.
> > > >
> > > > Well, they are orthogonal and what I want to say is, we should first
> > > > define the semantics of stop and state of the virtqueue.
> > > >
> > > > Such a facility could be accessed by either transport specific
> > > > method or admin virtqueue, it totally depends on the hardware
> > architecture of the vendor.
> > > >
> > > I find it hard to believe that a vendor can implement a CVQ but not AQ and
> > chose to expose tens of hundreds of registers.
> > > But maybe, it fits some specific hw.
> >
> > You can have a look at the ifcvf dpdk driver as an example.
> >
> Ifcvf is an example of using registers.
> It is not an answer why AQ is hard for it. :)

Well, it's an example of how vDPA is implemented. I think we agree
that for vDPA, vendors have the flexibility to implement their
perferrable datapath.

> virtio spec has definition of queue now and implementing yet another queue shouldn't be a problem.
>
> So far no one seem to have problem with the additional queue.
> So I take it as AQ is ok.
>
> > But another thing that is unrelated to hardware architecture is the nesting
> > support. Having admin virtqueue in a nesting environment looks like an
> > overkill. Presenting a register in L1 and map it to L0's admin should be good
> > enough.
> So may be a optimized interface can be added that fits nested env.
> At this point in time real users that we heard are interested in non-nested use cases. Let's enable them first.

That's fine. For nests, it's actually really easy, just adding an
interface within the existing transport should be sufficient.

>
>
> >
> > >
> > > I like to learn the advantages of such method other than simplicity.
> > >
> > > We can clearly that we are shifting away from such PCI registers with SIOV,
> > IMS and other scalable solutions.
> > > virtio drifting in reverse direction by introducing more registers as
> > transport.
> > > I expect it to an optional transport like AQ.
> >
> > Actually, I had a proposal of using admin virtqueue as a transport, it's
> > designed to be SIOV/IMS capable. And it's not hard to extend it with the
> > state/stop support etc.
> >
> > >
> > > > >
> > > > > Next would be to program hundreds of statistics of the 64 VQs
> > > > > through a
> > > > giant PCI config space register in some busy polling scheme.
> > > >
> > > > We don't need giant config space, and this method has been
> > > > implemented by some vDPA vendors.
> > > >
> > > There are tens of 64-bit counters per VQs. These needs to programmed on
> > destination side.
> > > Programming these via registers requires exposing them on the registers.
> > > In one of the proposals, I see them being queried via CVQ from the device.
> >
> > I didn't see a proposal like this. And I don't think querying general virtio state
> > like idx with a device specific CVQ is a good design.
> >
> My example was not for the idx. But for VQ statistics that is queried via CVQ.
>
> > >
> > > Programming them via cfg registers requires large cfg space or synchronous
> > programming until receiving ACK from it.
> > > This means one entry at a time...
> > >
> > > Programming them via CVQ needs replicate and align cmd values etc on all
> > device types. All duplicate and hard to maintain.
> > >
> > >
> > > > >
> > > > > I can clearly see how all these are inefficient for faster LM.
> > > > > We need an efficient AQ to proceed with at minimum.
> > > >
> > > > I'm fine with admin virtqueue, but the stop and state are orthogonal to
> > that.
> > > > And using admin virtqueue for stop/state will be more natural if we
> > > > use admin virtqueue as a transport.
> > > Ok.
> > > We should have defined it bit earlier that all vendors can use. :(
> >
> > I agree.
>
> I remember few months back, you acked in the weekly meeting that TC has approved the AQ direction.
> And we are still in this circle of debating the AQ.

I think not. Just to make sure we are on the same page, the proposal
here is for vDPA, and hope it can provide forward compatibility to
virtio. So in the context of vDPA, admin virtqueue is not a must.

Thanks

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Wed, Jun 15, 2022 at 8:10 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, June 1, 2022 11:54 PM
> >
> > On Thu, Jun 2, 2022 at 10:59 AM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Wednesday, June 1, 2022 10:00 PM
> > > >
> > > > On Thu, Jun 2, 2022 at 2:58 AM Parav Pandit <parav@nvidia.com> wrote:
> > > > >
> > > > >
> > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > Sent: Tuesday, May 31, 2022 10:42 PM
> > > > > >
> > > > > > Well, the ability to query the virtqueue state was proposed as
> > > > > > another feature (Eugenio, please correct me). This should be
> > > > > > sufficient for making virtio-net to be live migrated.
> > > > > >
> > > > > The device is stopped, it won't answer to this special vq config done
> > here.
> > > >
> > > > This depends on the definition of the stop. Any query to the device
> > > > state should be allowed otherwise it's meaningless for us.
> > > >
> > > > > Programming all of these using cfg registers doesn't scale for
> > > > > on-chip
> > > > memory and for the speed.
> > > >
> > > > Well, they are orthogonal and what I want to say is, we should first
> > > > define the semantics of stop and state of the virtqueue.
> > > >
> > > > Such a facility could be accessed by either transport specific
> > > > method or admin virtqueue, it totally depends on the hardware
> > architecture of the vendor.
> > > >
> > > I find it hard to believe that a vendor can implement a CVQ but not AQ and
> > chose to expose tens of hundreds of registers.
> > > But maybe, it fits some specific hw.
> >
> > You can have a look at the ifcvf dpdk driver as an example.
> >
> Ifcvf is an example of using registers.
> It is not an answer why AQ is hard for it. :)

Well, it's an example of how vDPA is implemented. I think we agree
that for vDPA, vendors have the flexibility to implement their
perferrable datapath.

> virtio spec has definition of queue now and implementing yet another queue shouldn't be a problem.
>
> So far no one seem to have problem with the additional queue.
> So I take it as AQ is ok.
>
> > But another thing that is unrelated to hardware architecture is the nesting
> > support. Having admin virtqueue in a nesting environment looks like an
> > overkill. Presenting a register in L1 and map it to L0's admin should be good
> > enough.
> So may be a optimized interface can be added that fits nested env.
> At this point in time real users that we heard are interested in non-nested use cases. Let's enable them first.

That's fine. For nests, it's actually really easy, just adding an
interface within the existing transport should be sufficient.

>
>
> >
> > >
> > > I like to learn the advantages of such method other than simplicity.
> > >
> > > We can clearly that we are shifting away from such PCI registers with SIOV,
> > IMS and other scalable solutions.
> > > virtio drifting in reverse direction by introducing more registers as
> > transport.
> > > I expect it to an optional transport like AQ.
> >
> > Actually, I had a proposal of using admin virtqueue as a transport, it's
> > designed to be SIOV/IMS capable. And it's not hard to extend it with the
> > state/stop support etc.
> >
> > >
> > > > >
> > > > > Next would be to program hundreds of statistics of the 64 VQs
> > > > > through a
> > > > giant PCI config space register in some busy polling scheme.
> > > >
> > > > We don't need giant config space, and this method has been
> > > > implemented by some vDPA vendors.
> > > >
> > > There are tens of 64-bit counters per VQs. These needs to programmed on
> > destination side.
> > > Programming these via registers requires exposing them on the registers.
> > > In one of the proposals, I see them being queried via CVQ from the device.
> >
> > I didn't see a proposal like this. And I don't think querying general virtio state
> > like idx with a device specific CVQ is a good design.
> >
> My example was not for the idx. But for VQ statistics that is queried via CVQ.
>
> > >
> > > Programming them via cfg registers requires large cfg space or synchronous
> > programming until receiving ACK from it.
> > > This means one entry at a time...
> > >
> > > Programming them via CVQ needs replicate and align cmd values etc on all
> > device types. All duplicate and hard to maintain.
> > >
> > >
> > > > >
> > > > > I can clearly see how all these are inefficient for faster LM.
> > > > > We need an efficient AQ to proceed with at minimum.
> > > >
> > > > I'm fine with admin virtqueue, but the stop and state are orthogonal to
> > that.
> > > > And using admin virtqueue for stop/state will be more natural if we
> > > > use admin virtqueue as a transport.
> > > Ok.
> > > We should have defined it bit earlier that all vendors can use. :(
> >
> > I agree.
>
> I remember few months back, you acked in the weekly meeting that TC has approved the AQ direction.
> And we are still in this circle of debating the AQ.

I think not. Just to make sure we are on the same page, the proposal
here is for vDPA, and hope it can provide forward compatibility to
virtio. So in the context of vDPA, admin virtqueue is not a must.

Thanks

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

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, June 14, 2022 9:29 PM
> 
> Well, it's an example of how vDPA is implemented. I think we agree that for
> vDPA, vendors have the flexibility to implement their perferrable datapath.
>
Yes for the vdpa level and for the virtio level.

> >
> > I remember few months back, you acked in the weekly meeting that TC has
> approved the AQ direction.
> > And we are still in this circle of debating the AQ.
> 
> I think not. Just to make sure we are on the same page, the proposal here is
> for vDPA, and hope it can provide forward compatibility to virtio. So in the
> context of vDPA, admin virtqueue is not a must.
In context of vdpa over virtio, an efficient transport interface is needed.
If AQ is not much any other interface such as hundreds to thousands of registers is not must either.

AQ is one interface proposed with multiple benefits.
I haven’t seen any other alternatives that delivers all the benefits.
Only one I have seen is synchronous config registers.

If you let vendors progress, handful of sensible interfaces can exist, each with different characteristics.
How would we proceed from here?

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit via Virtualization]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, June 14, 2022 9:29 PM
> 
> Well, it's an example of how vDPA is implemented. I think we agree that for
> vDPA, vendors have the flexibility to implement their perferrable datapath.
>
Yes for the vdpa level and for the virtio level.

> >
> > I remember few months back, you acked in the weekly meeting that TC has
> approved the AQ direction.
> > And we are still in this circle of debating the AQ.
> 
> I think not. Just to make sure we are on the same page, the proposal here is
> for vDPA, and hope it can provide forward compatibility to virtio. So in the
> context of vDPA, admin virtqueue is not a must.
In context of vdpa over virtio, an efficient transport interface is needed.
If AQ is not much any other interface such as hundreds to thousands of registers is not must either.

AQ is one interface proposed with multiple benefits.
I haven’t seen any other alternatives that delivers all the benefits.
Only one I have seen is synchronous config registers.

If you let vendors progress, handful of sensible interfaces can exist, each with different characteristics.
How would we proceed from here?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Fri, Jun 17, 2022 at 3:36 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, June 14, 2022 9:29 PM
> >
> > Well, it's an example of how vDPA is implemented. I think we agree that for
> > vDPA, vendors have the flexibility to implement their perferrable datapath.
> >
> Yes for the vdpa level and for the virtio level.
>
> > >
> > > I remember few months back, you acked in the weekly meeting that TC has
> > approved the AQ direction.
> > > And we are still in this circle of debating the AQ.
> >
> > I think not. Just to make sure we are on the same page, the proposal here is
> > for vDPA, and hope it can provide forward compatibility to virtio. So in the
> > context of vDPA, admin virtqueue is not a must.
> In context of vdpa over virtio, an efficient transport interface is needed.
> If AQ is not much any other interface such as hundreds to thousands of registers is not must either.
>
> AQ is one interface proposed with multiple benefits.
> I haven’t seen any other alternatives that delivers all the benefits.
> Only one I have seen is synchronous config registers.
>
> If you let vendors progress, handful of sensible interfaces can exist, each with different characteristics.
> How would we proceed from here?

I'm pretty fine with having admin virtqueue in the virtio spec. If you
remember, I've even submitted a proposal to use admin virtqueue as a
transport last year.

Let's just proceed in the virtio-dev list.

Thanks

Re: [PATCH v4 0/4] Implement vdpasim stop operation

[Jason Wang]

On Fri, Jun 17, 2022 at 3:36 AM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, June 14, 2022 9:29 PM
> >
> > Well, it's an example of how vDPA is implemented. I think we agree that for
> > vDPA, vendors have the flexibility to implement their perferrable datapath.
> >
> Yes for the vdpa level and for the virtio level.
>
> > >
> > > I remember few months back, you acked in the weekly meeting that TC has
> > approved the AQ direction.
> > > And we are still in this circle of debating the AQ.
> >
> > I think not. Just to make sure we are on the same page, the proposal here is
> > for vDPA, and hope it can provide forward compatibility to virtio. So in the
> > context of vDPA, admin virtqueue is not a must.
> In context of vdpa over virtio, an efficient transport interface is needed.
> If AQ is not much any other interface such as hundreds to thousands of registers is not must either.
>
> AQ is one interface proposed with multiple benefits.
> I haven’t seen any other alternatives that delivers all the benefits.
> Only one I have seen is synchronous config registers.
>
> If you let vendors progress, handful of sensible interfaces can exist, each with different characteristics.
> How would we proceed from here?

I'm pretty fine with having admin virtqueue in the virtio spec. If you
remember, I've even submitted a proposal to use admin virtqueue as a
transport last year.

Let's just proceed in the virtio-dev list.

Thanks

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

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Thursday, June 16, 2022 9:15 PM
> 
> On Fri, Jun 17, 2022 at 3:36 AM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Tuesday, June 14, 2022 9:29 PM
> > >
> > > Well, it's an example of how vDPA is implemented. I think we agree
> > > that for vDPA, vendors have the flexibility to implement their perferrable
> datapath.
> > >
> > Yes for the vdpa level and for the virtio level.
> >
> > > >
> > > > I remember few months back, you acked in the weekly meeting that
> > > > TC has
> > > approved the AQ direction.
> > > > And we are still in this circle of debating the AQ.
> > >
> > > I think not. Just to make sure we are on the same page, the proposal
> > > here is for vDPA, and hope it can provide forward compatibility to
> > > virtio. So in the context of vDPA, admin virtqueue is not a must.
> > In context of vdpa over virtio, an efficient transport interface is needed.
> > If AQ is not much any other interface such as hundreds to thousands of
> registers is not must either.
> >
> > AQ is one interface proposed with multiple benefits.
> > I haven’t seen any other alternatives that delivers all the benefits.
> > Only one I have seen is synchronous config registers.
> >
> > If you let vendors progress, handful of sensible interfaces can exist, each
> with different characteristics.
> > How would we proceed from here?
> 
> I'm pretty fine with having admin virtqueue in the virtio spec. If you
> remember, I've even submitted a proposal to use admin virtqueue as a
> transport last year.
> 
> Let's just proceed in the virtio-dev list.

o.k. thanks. I am aligned with your thoughts now.

RE: [PATCH v4 0/4] Implement vdpasim stop operation

[Parav Pandit via Virtualization]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Thursday, June 16, 2022 9:15 PM
> 
> On Fri, Jun 17, 2022 at 3:36 AM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Tuesday, June 14, 2022 9:29 PM
> > >
> > > Well, it's an example of how vDPA is implemented. I think we agree
> > > that for vDPA, vendors have the flexibility to implement their perferrable
> datapath.
> > >
> > Yes for the vdpa level and for the virtio level.
> >
> > > >
> > > > I remember few months back, you acked in the weekly meeting that
> > > > TC has
> > > approved the AQ direction.
> > > > And we are still in this circle of debating the AQ.
> > >
> > > I think not. Just to make sure we are on the same page, the proposal
> > > here is for vDPA, and hope it can provide forward compatibility to
> > > virtio. So in the context of vDPA, admin virtqueue is not a must.
> > In context of vdpa over virtio, an efficient transport interface is needed.
> > If AQ is not much any other interface such as hundreds to thousands of
> registers is not must either.
> >
> > AQ is one interface proposed with multiple benefits.
> > I haven’t seen any other alternatives that delivers all the benefits.
> > Only one I have seen is synchronous config registers.
> >
> > If you let vendors progress, handful of sensible interfaces can exist, each
> with different characteristics.
> > How would we proceed from here?
> 
> I'm pretty fine with having admin virtqueue in the virtio spec. If you
> remember, I've even submitted a proposal to use admin virtqueue as a
> transport last year.
> 
> Let's just proceed in the virtio-dev list.

o.k. thanks. I am aligned with your thoughts now.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization