[PATCH 0/4] Implement vdpasim stop operation

[PATCH 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).

Comments are welcome.

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_net.c |  3 +++
 drivers/vhost/vdpa.c                 | 31 ++++++++++++++++++++++++++++
 include/linux/vdpa.h                 |  6 ++++++
 include/uapi/linux/vhost.h           |  3 +++
 include/uapi/linux/vhost_types.h     |  2 ++
 7 files changed, 67 insertions(+)

-- 
2.27.0

[PATCH 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.27.0

[PATCH 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             | 13 +++++++++++++
 include/uapi/linux/vhost_types.h |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 1f1d1c425573..a325bc259afb 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;
@@ -577,6 +585,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
 			return -EFAULT;
 		if (features & ~VHOST_VDPA_BACKEND_FEATURES)
 			return -EOPNOTSUPP;
+		if ((features & VHOST_BACKEND_F_STOP) &&
+		     !vhost_vdpa_can_stop(v))
+			return -EOPNOTSUPP;
 		vhost_set_backend_features(&v->vdev, features);
 		return 0;
 	}
@@ -624,6 +635,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 |= 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.27.0

[PATCH 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 |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index a325bc259afb..da4a8c709bc1 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_to_user(argp, &stop, 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)
 {
@@ -649,6 +664,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_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..e7526968ab0c 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -171,4 +171,7 @@
 #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 */
+#define VHOST_STOP			_IOW(VHOST_VIRTIO, 0x7D, int)
+
 #endif
-- 
2.27.0

[PATCH 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_net.c |  3 +++
 3 files changed, 25 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_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.27.0

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

[kernel test robot]

Hi "Eugenio,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on next-20220520]
[cannot apply to horms-ipvs/master linus/master v5.18-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Eugenio-P-rez/Implement-vdpasim-stop-operation/20220521-012742
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: hexagon-randconfig-r041-20220519 (https://download.01.org/0day-ci/archive/20220521/202205211317.rqYkIW8B-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project e00cbbec06c08dc616a0d52a20f678b8fbd4e304)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/bd6562e0d85e8d689d30226bcc0f43246e27e4b5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Eugenio-P-rez/Implement-vdpasim-stop-operation/20220521-012742
        git checkout bd6562e0d85e8d689d30226bcc0f43246e27e4b5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/vhost/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/vhost/vdpa.c:493:25: warning: variable 'stop' is uninitialized when used here [-Wuninitialized]
           return ops->stop(vdpa, stop);
                                  ^~~~
   drivers/vhost/vdpa.c:485:10: note: initialize the variable 'stop' to silence this warning
           int stop;
                   ^
                    = 0
   1 warning generated.


vim +/stop +493 drivers/vhost/vdpa.c

   480	
   481	static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
   482	{
   483		struct vdpa_device *vdpa = v->vdpa;
   484		const struct vdpa_config_ops *ops = vdpa->config;
   485		int stop;
   486	
   487		if (!ops->stop)
   488			return -EOPNOTSUPP;
   489	
   490		if (copy_to_user(argp, &stop, sizeof(stop)))
   491			return -EFAULT;
   492	
 > 493		return ops->stop(vdpa, stop);
   494	}
   495	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi "Eugenio,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on next-20220520]
[cannot apply to horms-ipvs/master linus/master v5.18-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Eugenio-P-rez/Implement-vdpasim-stop-operation/20220521-012742
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: hexagon-randconfig-r041-20220519 (https://download.01.org/0day-ci/archive/20220521/202205211317.rqYkIW8B-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project e00cbbec06c08dc616a0d52a20f678b8fbd4e304)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/bd6562e0d85e8d689d30226bcc0f43246e27e4b5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Eugenio-P-rez/Implement-vdpasim-stop-operation/20220521-012742
        git checkout bd6562e0d85e8d689d30226bcc0f43246e27e4b5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/vhost/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/vhost/vdpa.c:493:25: warning: variable 'stop' is uninitialized when used here [-Wuninitialized]
           return ops->stop(vdpa, stop);
                                  ^~~~
   drivers/vhost/vdpa.c:485:10: note: initialize the variable 'stop' to silence this warning
           int stop;
                   ^
                    = 0
   1 warning generated.


vim +/stop +493 drivers/vhost/vdpa.c

   480	
   481	static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
   482	{
   483		struct vdpa_device *vdpa = v->vdpa;
   484		const struct vdpa_config_ops *ops = vdpa->config;
   485		int stop;
   486	
   487		if (!ops->stop)
   488			return -EOPNOTSUPP;
   489	
   490		if (copy_to_user(argp, &stop, sizeof(stop)))
   491			return -EFAULT;
   492	
 > 493		return ops->stop(vdpa, stop);
   494	}
   495	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Martin Habets]

On Fri, May 20, 2022 at 07:23:24PM +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 |  3 +++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index a325bc259afb..da4a8c709bc1 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_to_user(argp, &stop, sizeof(stop)))

You want to use copy_from_user() here.

Martin

> +		return -EFAULT;
> +
> +	return ops->stop(vdpa, stop);
> +}
> +
>  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>  				   void __user *argp)
>  {
> @@ -649,6 +664,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_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..e7526968ab0c 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -171,4 +171,7 @@
>  #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 */
> +#define VHOST_STOP			_IOW(VHOST_VIRTIO, 0x7D, int)
> +
>  #endif
> -- 
> 2.27.0

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

[Si-Wei Liu]

On 5/20/2022 10:23 AM, 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)
> + *				Returns integer: success (0) or error (< 0)
Is this uAPI meant to address all use cases described in the full blown 
_F_STOP virtio spec proposal, such as:

--------------%<--------------

...... the device MUST finish any in flight
operations after the driver writes STOP.  Depending on the device, it 
can do it
in many ways as long as the driver can recover its normal operation if it
resumes the device without the need of resetting it:

- Drain and wait for the completion of all pending requests until a
   convenient avail descriptor. Ignore any other posterior descriptor.
- Return a device-specific failure for these descriptors, so the driver
   can choose to retry or to cancel them.
- Mark them as done even if they are not, if the kind of device can
   assume to lose them.
--------------%<--------------

E.g. do I assume correctly all in flight requests are flushed after 
return from this uAPI call? Or some of pending requests may be subject 
to loss or failure? How does the caller/user specify these various 
options (if there are) for device stop?

BTW, it would be nice to add the corresponding support to vdpa_sim_blk 
as well to demo the stop handling. To just show it on vdpa-sim-net IMHO 
is perhaps not so convincing.

-Siwei

>    * @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);

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

[Si-Wei Liu]

On 5/20/2022 10:23 AM, 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)
> + *				Returns integer: success (0) or error (< 0)
Is this uAPI meant to address all use cases described in the full blown 
_F_STOP virtio spec proposal, such as:

--------------%<--------------

...... the device MUST finish any in flight
operations after the driver writes STOP.  Depending on the device, it 
can do it
in many ways as long as the driver can recover its normal operation if it
resumes the device without the need of resetting it:

- Drain and wait for the completion of all pending requests until a
   convenient avail descriptor. Ignore any other posterior descriptor.
- Return a device-specific failure for these descriptors, so the driver
   can choose to retry or to cancel them.
- Mark them as done even if they are not, if the kind of device can
   assume to lose them.
--------------%<--------------

E.g. do I assume correctly all in flight requests are flushed after 
return from this uAPI call? Or some of pending requests may be subject 
to loss or failure? How does the caller/user specify these various 
options (if there are) for device stop?

BTW, it would be nice to add the corresponding support to vdpa_sim_blk 
as well to demo the stop handling. To just show it on vdpa-sim-net IMHO 
is perhaps not so convincing.

-Siwei

>    * @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);

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

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

[Si-Wei Liu]

On 5/20/2022 10:23 AM, Eugenio Pérez wrote:
> 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             | 13 +++++++++++++
>   include/uapi/linux/vhost_types.h |  2 ++
>   2 files changed, 15 insertions(+)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 1f1d1c425573..a325bc259afb 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;
> @@ -577,6 +585,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
>   			return -EFAULT;
>   		if (features & ~VHOST_VDPA_BACKEND_FEATURES)
>   			return -EOPNOTSUPP;
> +		if ((features & VHOST_BACKEND_F_STOP) &&
VHOST_BACKEND_F_STOP is not part of VHOST_VDPA_BACKEND_FEATURES. There's 
no chance for VHOST_BACKEND_F_STOP to get here.

-Siwei
> +		     !vhost_vdpa_can_stop(v))
> +			return -EOPNOTSUPP;
>   		vhost_set_backend_features(&v->vdev, features);
>   		return 0;
>   	}
> @@ -624,6 +635,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 |= 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

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

[Si-Wei Liu]

On 5/20/2022 10:23 AM, Eugenio Pérez wrote:
> 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             | 13 +++++++++++++
>   include/uapi/linux/vhost_types.h |  2 ++
>   2 files changed, 15 insertions(+)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 1f1d1c425573..a325bc259afb 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;
> @@ -577,6 +585,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
>   			return -EFAULT;
>   		if (features & ~VHOST_VDPA_BACKEND_FEATURES)
>   			return -EOPNOTSUPP;
> +		if ((features & VHOST_BACKEND_F_STOP) &&
VHOST_BACKEND_F_STOP is not part of VHOST_VDPA_BACKEND_FEATURES. There's 
no chance for VHOST_BACKEND_F_STOP to get here.

-Siwei
> +		     !vhost_vdpa_can_stop(v))
> +			return -EOPNOTSUPP;
>   		vhost_set_backend_features(&v->vdev, features);
>   		return 0;
>   	}
> @@ -624,6 +635,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 |= 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

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

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

[Eugenio Perez Martin]

On Sat, May 21, 2022 at 10:36 AM Martin Habets <habetsm.xilinx@gmail.com> wrote:
>
> On Fri, May 20, 2022 at 07:23:24PM +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 |  3 +++
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index a325bc259afb..da4a8c709bc1 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_to_user(argp, &stop, sizeof(stop)))
>
> You want to use copy_from_user() here.
>

Hi Martin,

You're right, I'll resend a new version with this fixed. Thanks!

> Martin
>
> > +             return -EFAULT;
> > +
> > +     return ops->stop(vdpa, stop);
> > +}
> > +
> >  static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
> >                                  void __user *argp)
> >  {
> > @@ -649,6 +664,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_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..e7526968ab0c 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -171,4 +171,7 @@
> >  #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 */
> > +#define VHOST_STOP                   _IOW(VHOST_VIRTIO, 0x7D, int)
> > +
> >  #endif
> > --
> > 2.27.0
>

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

[Stefano Garzarella]

On Fri, May 20, 2022 at 07:23:25PM +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_net.c |  3 +++
> 3 files changed, 25 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_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;
>+

It would be nice to do the same for vdpa_sim_blk as well.

Thanks,
Stefano

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

[Stefano Garzarella]

On Fri, May 20, 2022 at 07:23:25PM +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_net.c |  3 +++
> 3 files changed, 25 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_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;
>+

It would be nice to do the same for vdpa_sim_blk as well.

Thanks,
Stefano

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

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

[Eugenio Perez Martin]

On Sat, May 21, 2022 at 12:25 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 5/20/2022 10:23 AM, Eugenio Pérez wrote:
> > 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             | 13 +++++++++++++
> >   include/uapi/linux/vhost_types.h |  2 ++
> >   2 files changed, 15 insertions(+)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index 1f1d1c425573..a325bc259afb 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;
> > @@ -577,6 +585,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
> >                       return -EFAULT;
> >               if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> >                       return -EOPNOTSUPP;
> > +             if ((features & VHOST_BACKEND_F_STOP) &&
> VHOST_BACKEND_F_STOP is not part of VHOST_VDPA_BACKEND_FEATURES. There's
> no chance for VHOST_BACKEND_F_STOP to get here.
>

That's right. I think I missed to backport your patches about set
backend_features only once in my testing.

I will re-test with the latest qemu master, thanks!

> -Siwei
> > +                  !vhost_vdpa_can_stop(v))
> > +                     return -EOPNOTSUPP;
> >               vhost_set_backend_features(&v->vdev, features);
> >               return 0;
> >       }
> > @@ -624,6 +635,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 |= 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
>

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

[Eugenio Perez Martin]

On Mon, May 23, 2022 at 10:28 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, May 20, 2022 at 07:23:25PM +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_net.c |  3 +++
> > 3 files changed, 25 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_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;
> >+
>
> It would be nice to do the same for vdpa_sim_blk as well.
>

Agree, it will be added in the next revision. If not, blk presents an
invalid backend feature bit.

Thanks!

> Thanks,
> Stefano
>

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

[Eugenio Perez Martin]

On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 5/20/2022 10:23 AM, 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)
> > + *                           Returns integer: success (0) or error (< 0)
> Is this uAPI meant to address all use cases described in the full blown
> _F_STOP virtio spec proposal, such as:
>
> --------------%<--------------
>
> ...... the device MUST finish any in flight
> operations after the driver writes STOP.  Depending on the device, it
> can do it
> in many ways as long as the driver can recover its normal operation if it
> resumes the device without the need of resetting it:
>
> - Drain and wait for the completion of all pending requests until a
>    convenient avail descriptor. Ignore any other posterior descriptor.
> - Return a device-specific failure for these descriptors, so the driver
>    can choose to retry or to cancel them.
> - Mark them as done even if they are not, if the kind of device can
>    assume to lose them.
> --------------%<--------------
>

Right, this is totally underspecified in this series.

I'll expand on it in the next version, but that text proposed to
virtio-comment was complicated and misleading. I find better to get
the previous version description. Would the next description work?

```
After the return of ioctl, 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.

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

Thanks for pointing it out!





> E.g. do I assume correctly all in flight requests are flushed after
> return from this uAPI call? Or some of pending requests may be subject
> to loss or failure? How does the caller/user specify these various
> options (if there are) for device stop?
>
> BTW, it would be nice to add the corresponding support to vdpa_sim_blk
> as well to demo the stop handling. To just show it on vdpa-sim-net IMHO
> is perhaps not so convincing.
>
> -Siwei
>
> >    * @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);
>

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

[Si-Wei Liu]

On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:
> On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 5/20/2022 10:23 AM, 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)
>>> + *                           Returns integer: success (0) or error (< 0)
>> Is this uAPI meant to address all use cases described in the full blown
>> _F_STOP virtio spec proposal, such as:
>>
>> --------------%<--------------
>>
>> ...... the device MUST finish any in flight
>> operations after the driver writes STOP.  Depending on the device, it
>> can do it
>> in many ways as long as the driver can recover its normal operation if it
>> resumes the device without the need of resetting it:
>>
>> - Drain and wait for the completion of all pending requests until a
>>     convenient avail descriptor. Ignore any other posterior descriptor.
>> - Return a device-specific failure for these descriptors, so the driver
>>     can choose to retry or to cancel them.
>> - Mark them as done even if they are not, if the kind of device can
>>     assume to lose them.
>> --------------%<--------------
>>
> Right, this is totally underspecified in this series.
>
> I'll expand on it in the next version, but that text proposed to
> virtio-comment was complicated and misleading. I find better to get
> the previous version description. Would the next description work?
>
> ```
> After the return of ioctl, 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)
Hmmm, "possible device specific states" is a bit vague. Does it require 
the device to save any device internal state that is not defined in the 
virtio spec - such as any failed in-flight requests to resubmit upon 
resume? Or you would lean on SVQ to intercept it in depth and save it 
with some other means? I think network device also has internal state 
such as flow steering state that needs bookkeeping as well.

A follow-up question is what is the use of the `stop` argument of false, 
does it require the device to support resume? I seem to recall this is 
something to abandon in favor of device reset plus setting queue 
base/addr after. Or it's just a optional feature that may be device 
specific (if one can do so in simple way).

-Siwei

>   that is required
> for restoring in the future.
>
> In the future, we will provide features similar to VHOST_USER_GET_INFLIGHT_FD
> so the device can save pending operations.
> ```
>
> Thanks for pointing it out!
>
>
>
>
>
>> E.g. do I assume correctly all in flight requests are flushed after
>> return from this uAPI call? Or some of pending requests may be subject
>> to loss or failure? How does the caller/user specify these various
>> options (if there are) for device stop?
>>
>> BTW, it would be nice to add the corresponding support to vdpa_sim_blk
>> as well to demo the stop handling. To just show it on vdpa-sim-net IMHO
>> is perhaps not so convincing.
>>
>> -Siwei
>>
>>>     * @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);

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

[Si-Wei Liu]

On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:
> On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 5/20/2022 10:23 AM, 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)
>>> + *                           Returns integer: success (0) or error (< 0)
>> Is this uAPI meant to address all use cases described in the full blown
>> _F_STOP virtio spec proposal, such as:
>>
>> --------------%<--------------
>>
>> ...... the device MUST finish any in flight
>> operations after the driver writes STOP.  Depending on the device, it
>> can do it
>> in many ways as long as the driver can recover its normal operation if it
>> resumes the device without the need of resetting it:
>>
>> - Drain and wait for the completion of all pending requests until a
>>     convenient avail descriptor. Ignore any other posterior descriptor.
>> - Return a device-specific failure for these descriptors, so the driver
>>     can choose to retry or to cancel them.
>> - Mark them as done even if they are not, if the kind of device can
>>     assume to lose them.
>> --------------%<--------------
>>
> Right, this is totally underspecified in this series.
>
> I'll expand on it in the next version, but that text proposed to
> virtio-comment was complicated and misleading. I find better to get
> the previous version description. Would the next description work?
>
> ```
> After the return of ioctl, 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)
Hmmm, "possible device specific states" is a bit vague. Does it require 
the device to save any device internal state that is not defined in the 
virtio spec - such as any failed in-flight requests to resubmit upon 
resume? Or you would lean on SVQ to intercept it in depth and save it 
with some other means? I think network device also has internal state 
such as flow steering state that needs bookkeeping as well.

A follow-up question is what is the use of the `stop` argument of false, 
does it require the device to support resume? I seem to recall this is 
something to abandon in favor of device reset plus setting queue 
base/addr after. Or it's just a optional feature that may be device 
specific (if one can do so in simple way).

-Siwei

>   that is required
> for restoring in the future.
>
> In the future, we will provide features similar to VHOST_USER_GET_INFLIGHT_FD
> so the device can save pending operations.
> ```
>
> Thanks for pointing it out!
>
>
>
>
>
>> E.g. do I assume correctly all in flight requests are flushed after
>> return from this uAPI call? Or some of pending requests may be subject
>> to loss or failure? How does the caller/user specify these various
>> options (if there are) for device stop?
>>
>> BTW, it would be nice to add the corresponding support to vdpa_sim_blk
>> as well to demo the stop handling. To just show it on vdpa-sim-net IMHO
>> is perhaps not so convincing.
>>
>> -Siwei
>>
>>>     * @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);

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

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

[Si-Wei Liu]

On 5/23/2022 4:54 PM, Si-Wei Liu wrote:
>
>
> On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:
>> On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> 
>> wrote:
>>>
>>>
>>> On 5/20/2022 10:23 AM, 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)
>>>> + *                           Returns integer: success (0) or error 
>>>> (< 0)
>>> Is this uAPI meant to address all use cases described in the full blown
>>> _F_STOP virtio spec proposal, such as:
>>>
>>> --------------%<--------------
>>>
>>> ...... the device MUST finish any in flight
>>> operations after the driver writes STOP.  Depending on the device, it
>>> can do it
>>> in many ways as long as the driver can recover its normal operation 
>>> if it
>>> resumes the device without the need of resetting it:
>>>
>>> - Drain and wait for the completion of all pending requests until a
>>>     convenient avail descriptor. Ignore any other posterior descriptor.
>>> - Return a device-specific failure for these descriptors, so the driver
>>>     can choose to retry or to cancel them.
>>> - Mark them as done even if they are not, if the kind of device can
>>>     assume to lose them.
>>> --------------%<--------------
>>>
>> Right, this is totally underspecified in this series.
>>
>> I'll expand on it in the next version, but that text proposed to
>> virtio-comment was complicated and misleading. I find better to get
>> the previous version description. Would the next description work?
>>
>> ```
>> After the return of ioctl, 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)
> Hmmm, "possible device specific states" is a bit vague. Does it 
> require the device to save any device internal state that is not 
> defined in the virtio spec - such as any failed in-flight requests to 
> resubmit upon resume? Or you would lean on SVQ to intercept it in 
> depth and save it with some other means? I think network device also 
> has internal state such as flow steering state that needs bookkeeping 
> as well.
Noted that I understand you may introduce additional feature call 
similar to VHOST_USER_GET_INFLIGHT_FD for (failed) in-flight request, 
but since that's is a get interface, I assume the actual state 
preserving should still take place in this STOP call.

-Siwei

>
> A follow-up question is what is the use of the `stop` argument of 
> false, does it require the device to support resume? I seem to recall 
> this is something to abandon in favor of device reset plus setting 
> queue base/addr after. Or it's just a optional feature that may be 
> device specific (if one can do so in simple way).
>
> -Siwei
>
>>   that is required
>> for restoring in the future.
>>
>> In the future, we will provide features similar to 
>> VHOST_USER_GET_INFLIGHT_FD
>> so the device can save pending operations.
>> ```
>>
>> Thanks for pointing it out!
>>
>>
>>
>>
>>
>>> E.g. do I assume correctly all in flight requests are flushed after
>>> return from this uAPI call? Or some of pending requests may be subject
>>> to loss or failure? How does the caller/user specify these various
>>> options (if there are) for device stop?
>>>
>>> BTW, it would be nice to add the corresponding support to vdpa_sim_blk
>>> as well to demo the stop handling. To just show it on vdpa-sim-net IMHO
>>> is perhaps not so convincing.
>>>
>>> -Siwei
>>>
>>>>     * @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);
>

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

[Si-Wei Liu]

On 5/23/2022 4:54 PM, Si-Wei Liu wrote:
>
>
> On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:
>> On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> 
>> wrote:
>>>
>>>
>>> On 5/20/2022 10:23 AM, 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)
>>>> + *                           Returns integer: success (0) or error 
>>>> (< 0)
>>> Is this uAPI meant to address all use cases described in the full blown
>>> _F_STOP virtio spec proposal, such as:
>>>
>>> --------------%<--------------
>>>
>>> ...... the device MUST finish any in flight
>>> operations after the driver writes STOP.  Depending on the device, it
>>> can do it
>>> in many ways as long as the driver can recover its normal operation 
>>> if it
>>> resumes the device without the need of resetting it:
>>>
>>> - Drain and wait for the completion of all pending requests until a
>>>     convenient avail descriptor. Ignore any other posterior descriptor.
>>> - Return a device-specific failure for these descriptors, so the driver
>>>     can choose to retry or to cancel them.
>>> - Mark them as done even if they are not, if the kind of device can
>>>     assume to lose them.
>>> --------------%<--------------
>>>
>> Right, this is totally underspecified in this series.
>>
>> I'll expand on it in the next version, but that text proposed to
>> virtio-comment was complicated and misleading. I find better to get
>> the previous version description. Would the next description work?
>>
>> ```
>> After the return of ioctl, 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)
> Hmmm, "possible device specific states" is a bit vague. Does it 
> require the device to save any device internal state that is not 
> defined in the virtio spec - such as any failed in-flight requests to 
> resubmit upon resume? Or you would lean on SVQ to intercept it in 
> depth and save it with some other means? I think network device also 
> has internal state such as flow steering state that needs bookkeeping 
> as well.
Noted that I understand you may introduce additional feature call 
similar to VHOST_USER_GET_INFLIGHT_FD for (failed) in-flight request, 
but since that's is a get interface, I assume the actual state 
preserving should still take place in this STOP call.

-Siwei

>
> A follow-up question is what is the use of the `stop` argument of 
> false, does it require the device to support resume? I seem to recall 
> this is something to abandon in favor of device reset plus setting 
> queue base/addr after. Or it's just a optional feature that may be 
> device specific (if one can do so in simple way).
>
> -Siwei
>
>>   that is required
>> for restoring in the future.
>>
>> In the future, we will provide features similar to 
>> VHOST_USER_GET_INFLIGHT_FD
>> so the device can save pending operations.
>> ```
>>
>> Thanks for pointing it out!
>>
>>
>>
>>
>>
>>> E.g. do I assume correctly all in flight requests are flushed after
>>> return from this uAPI call? Or some of pending requests may be subject
>>> to loss or failure? How does the caller/user specify these various
>>> options (if there are) for device stop?
>>>
>>> BTW, it would be nice to add the corresponding support to vdpa_sim_blk
>>> as well to demo the stop handling. To just show it on vdpa-sim-net IMHO
>>> is perhaps not so convincing.
>>>
>>> -Siwei
>>>
>>>>     * @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);
>

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

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

[Jason Wang]

在 2022/5/24 08:01, Si-Wei Liu 写道:
>
>
> On 5/23/2022 4:54 PM, Si-Wei Liu wrote:
>>
>>
>> On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:
>>> On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> 
>>> wrote:
>>>>
>>>>
>>>> On 5/20/2022 10:23 AM, 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)
>>>>> + *                           Returns integer: success (0) or 
>>>>> error (< 0)
>>>> Is this uAPI meant to address all use cases described in the full 
>>>> blown
>>>> _F_STOP virtio spec proposal, such as:
>>>>
>>>> --------------%<--------------
>>>>
>>>> ...... the device MUST finish any in flight
>>>> operations after the driver writes STOP.  Depending on the device, it
>>>> can do it
>>>> in many ways as long as the driver can recover its normal operation 
>>>> if it
>>>> resumes the device without the need of resetting it:
>>>>
>>>> - Drain and wait for the completion of all pending requests until a
>>>>     convenient avail descriptor. Ignore any other posterior 
>>>> descriptor.
>>>> - Return a device-specific failure for these descriptors, so the 
>>>> driver
>>>>     can choose to retry or to cancel them.
>>>> - Mark them as done even if they are not, if the kind of device can
>>>>     assume to lose them.
>>>> --------------%<--------------
>>>>
>>> Right, this is totally underspecified in this series.
>>>
>>> I'll expand on it in the next version, but that text proposed to
>>> virtio-comment was complicated and misleading. I find better to get
>>> the previous version description. Would the next description work?
>>>
>>> ```
>>> After the return of ioctl, 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)
>> Hmmm, "possible device specific states" is a bit vague. Does it 
>> require the device to save any device internal state that is not 
>> defined in the virtio spec - such as any failed in-flight requests to 
>> resubmit upon resume? Or you would lean on SVQ to intercept it in 
>> depth and save it with some other means? I think network device also 
>> has internal state such as flow steering state that needs bookkeeping 
>> as well.
> Noted that I understand you may introduce additional feature call 
> similar to VHOST_USER_GET_INFLIGHT_FD for (failed) in-flight request, 
> but since that's is a get interface, I assume the actual state 
> preserving should still take place in this STOP call.
>

Yes, I think so.


> -Siwei
>
>>
>> A follow-up question is what is the use of the `stop` argument of 
>> false, does it require the device to support resume? 


Yes, this is required by the hypervisor e.g for Qemu it supports vm 
stop/resume.


>> I seem to recall this is something to abandon in favor of device 
>> reset plus setting queue base/addr after. Or it's just a optional 
>> feature that may be device specific (if one can do so in simple way).


Rest is more like a workarond consider we don't have a stop API. 
Consider we don't add stop at the beginning, it can only be an optional 
feature.

Thanks


>>
>> -Siwei
>>
>>>   that is required
>>> for restoring in the future.
>>>
>>> In the future, we will provide features similar to 
>>> VHOST_USER_GET_INFLIGHT_FD
>>> so the device can save pending operations.
>>> ```
>>>
>>> Thanks for pointing it out!
>>>
>>>
>>>
>>>
>>>
>>>> E.g. do I assume correctly all in flight requests are flushed after
>>>> return from this uAPI call? Or some of pending requests may be subject
>>>> to loss or failure? How does the caller/user specify these various
>>>> options (if there are) for device stop?
>>>>
>>>> BTW, it would be nice to add the corresponding support to vdpa_sim_blk
>>>> as well to demo the stop handling. To just show it on vdpa-sim-net 
>>>> IMHO
>>>> is perhaps not so convincing.
>>>>
>>>> -Siwei
>>>>
>>>>>     * @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);
>>
>

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

[Jason Wang]

在 2022/5/24 08:01, Si-Wei Liu 写道:
>
>
> On 5/23/2022 4:54 PM, Si-Wei Liu wrote:
>>
>>
>> On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:
>>> On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> 
>>> wrote:
>>>>
>>>>
>>>> On 5/20/2022 10:23 AM, 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)
>>>>> + *                           Returns integer: success (0) or 
>>>>> error (< 0)
>>>> Is this uAPI meant to address all use cases described in the full 
>>>> blown
>>>> _F_STOP virtio spec proposal, such as:
>>>>
>>>> --------------%<--------------
>>>>
>>>> ...... the device MUST finish any in flight
>>>> operations after the driver writes STOP.  Depending on the device, it
>>>> can do it
>>>> in many ways as long as the driver can recover its normal operation 
>>>> if it
>>>> resumes the device without the need of resetting it:
>>>>
>>>> - Drain and wait for the completion of all pending requests until a
>>>>     convenient avail descriptor. Ignore any other posterior 
>>>> descriptor.
>>>> - Return a device-specific failure for these descriptors, so the 
>>>> driver
>>>>     can choose to retry or to cancel them.
>>>> - Mark them as done even if they are not, if the kind of device can
>>>>     assume to lose them.
>>>> --------------%<--------------
>>>>
>>> Right, this is totally underspecified in this series.
>>>
>>> I'll expand on it in the next version, but that text proposed to
>>> virtio-comment was complicated and misleading. I find better to get
>>> the previous version description. Would the next description work?
>>>
>>> ```
>>> After the return of ioctl, 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)
>> Hmmm, "possible device specific states" is a bit vague. Does it 
>> require the device to save any device internal state that is not 
>> defined in the virtio spec - such as any failed in-flight requests to 
>> resubmit upon resume? Or you would lean on SVQ to intercept it in 
>> depth and save it with some other means? I think network device also 
>> has internal state such as flow steering state that needs bookkeeping 
>> as well.
> Noted that I understand you may introduce additional feature call 
> similar to VHOST_USER_GET_INFLIGHT_FD for (failed) in-flight request, 
> but since that's is a get interface, I assume the actual state 
> preserving should still take place in this STOP call.
>

Yes, I think so.


> -Siwei
>
>>
>> A follow-up question is what is the use of the `stop` argument of 
>> false, does it require the device to support resume? 


Yes, this is required by the hypervisor e.g for Qemu it supports vm 
stop/resume.


>> I seem to recall this is something to abandon in favor of device 
>> reset plus setting queue base/addr after. Or it's just a optional 
>> feature that may be device specific (if one can do so in simple way).


Rest is more like a workarond consider we don't have a stop API. 
Consider we don't add stop at the beginning, it can only be an optional 
feature.

Thanks


>>
>> -Siwei
>>
>>>   that is required
>>> for restoring in the future.
>>>
>>> In the future, we will provide features similar to 
>>> VHOST_USER_GET_INFLIGHT_FD
>>> so the device can save pending operations.
>>> ```
>>>
>>> Thanks for pointing it out!
>>>
>>>
>>>
>>>
>>>
>>>> E.g. do I assume correctly all in flight requests are flushed after
>>>> return from this uAPI call? Or some of pending requests may be subject
>>>> to loss or failure? How does the caller/user specify these various
>>>> options (if there are) for device stop?
>>>>
>>>> BTW, it would be nice to add the corresponding support to vdpa_sim_blk
>>>> as well to demo the stop handling. To just show it on vdpa-sim-net 
>>>> IMHO
>>>> is perhaps not so convincing.
>>>>
>>>> -Siwei
>>>>
>>>>>     * @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);
>>
>

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

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

[Stefano Garzarella]

On Mon, May 23, 2022 at 09:20:14PM +0200, Eugenio Perez Martin wrote:
>On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>>
>> On 5/20/2022 10:23 AM, 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)
>> > + *                           Returns integer: success (0) or error (< 0)
>> Is this uAPI meant to address all use cases described in the full blown
>> _F_STOP virtio spec proposal, such as:
>>
>> --------------%<--------------
>>
>> ...... the device MUST finish any in flight
>> operations after the driver writes STOP.  Depending on the device, it
>> can do it
>> in many ways as long as the driver can recover its normal operation 
>> if it
>> resumes the device without the need of resetting it:
>>
>> - Drain and wait for the completion of all pending requests until a
>>    convenient avail descriptor. Ignore any other posterior descriptor.
>> - Return a device-specific failure for these descriptors, so the driver
>>    can choose to retry or to cancel them.
>> - Mark them as done even if they are not, if the kind of device can
>>    assume to lose them.
>> --------------%<--------------
>>
>
>Right, this is totally underspecified in this series.
>
>I'll expand on it in the next version, but that text proposed to
>virtio-comment was complicated and misleading. I find better to get
>the previous version description. Would the next description work?
>
>```
>After the return of ioctl, 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.

For block devices wait for all in-flight requests could take several 
time.

Could this be a problem if the caller gets stuck on this ioctl?

If it could be a problem, maybe we should use an eventfd to signal that 
the device is successfully stopped.

Thanks,
Stefano

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

[Stefano Garzarella]

On Mon, May 23, 2022 at 09:20:14PM +0200, Eugenio Perez Martin wrote:
>On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>>
>> On 5/20/2022 10:23 AM, 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)
>> > + *                           Returns integer: success (0) or error (< 0)
>> Is this uAPI meant to address all use cases described in the full blown
>> _F_STOP virtio spec proposal, such as:
>>
>> --------------%<--------------
>>
>> ...... the device MUST finish any in flight
>> operations after the driver writes STOP.  Depending on the device, it
>> can do it
>> in many ways as long as the driver can recover its normal operation 
>> if it
>> resumes the device without the need of resetting it:
>>
>> - Drain and wait for the completion of all pending requests until a
>>    convenient avail descriptor. Ignore any other posterior descriptor.
>> - Return a device-specific failure for these descriptors, so the driver
>>    can choose to retry or to cancel them.
>> - Mark them as done even if they are not, if the kind of device can
>>    assume to lose them.
>> --------------%<--------------
>>
>
>Right, this is totally underspecified in this series.
>
>I'll expand on it in the next version, but that text proposed to
>virtio-comment was complicated and misleading. I find better to get
>the previous version description. Would the next description work?
>
>```
>After the return of ioctl, 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.

For block devices wait for all in-flight requests could take several 
time.

Could this be a problem if the caller gets stuck on this ioctl?

If it could be a problem, maybe we should use an eventfd to signal that 
the device is successfully stopped.

Thanks,
Stefano

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

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

[Eugenio Perez Martin]

On Tue, May 24, 2022 at 2:01 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 5/23/2022 4:54 PM, Si-Wei Liu wrote:
> >
> >
> > On 5/23/2022 12:20 PM, Eugenio Perez Martin wrote:
> >> On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com>
> >> wrote:
> >>>
> >>>
> >>> On 5/20/2022 10:23 AM, 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)
> >>>> + *                           Returns integer: success (0) or error
> >>>> (< 0)
> >>> Is this uAPI meant to address all use cases described in the full blown
> >>> _F_STOP virtio spec proposal, such as:
> >>>
> >>> --------------%<--------------
> >>>
> >>> ...... the device MUST finish any in flight
> >>> operations after the driver writes STOP.  Depending on the device, it
> >>> can do it
> >>> in many ways as long as the driver can recover its normal operation
> >>> if it
> >>> resumes the device without the need of resetting it:
> >>>
> >>> - Drain and wait for the completion of all pending requests until a
> >>>     convenient avail descriptor. Ignore any other posterior descriptor.
> >>> - Return a device-specific failure for these descriptors, so the driver
> >>>     can choose to retry or to cancel them.
> >>> - Mark them as done even if they are not, if the kind of device can
> >>>     assume to lose them.
> >>> --------------%<--------------
> >>>
> >> Right, this is totally underspecified in this series.
> >>
> >> I'll expand on it in the next version, but that text proposed to
> >> virtio-comment was complicated and misleading. I find better to get
> >> the previous version description. Would the next description work?
> >>
> >> ```
> >> After the return of ioctl, 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)
> > Hmmm, "possible device specific states" is a bit vague. Does it
> > require the device to save any device internal state that is not
> > defined in the virtio spec - such as any failed in-flight requests to
> > resubmit upon resume?

I'd let that be device-specific. For example, the net simulator
doesn't need to store them, since it cannot stop while processing
buffers. Other net devices can also decide to simply drop or re-submit
tx frames.

I can check for the block simulator if that's possible too. For
hardware vdpa block devices, this should be combined with the future
"get inflight buffers" call for sure.

> > Or you would lean on SVQ to intercept it in
> > depth and save it with some other means? I think network device also
> > has internal state such as flow steering state that needs bookkeeping
> > as well.

Yes, for state set by the control vq a permanent SVQ is used only for
the cvq. For other things like config space vdpa already presents an
emulated one to the guest, so we're safe in that regard.

> Noted that I understand you may introduce additional feature call
> similar to VHOST_USER_GET_INFLIGHT_FD for (failed) in-flight request,
> but since that's is a get interface, I assume the actual state
> preserving should still take place in this STOP call.
>

Right. I'll add all of this to the proposal.

Thanks!

> -Siwei
>
> >
> > A follow-up question is what is the use of the `stop` argument of
> > false, does it require the device to support resume? I seem to recall
> > this is something to abandon in favor of device reset plus setting
> > queue base/addr after. Or it's just a optional feature that may be
> > device specific (if one can do so in simple way).
> >
> > -Siwei
> >
> >>   that is required
> >> for restoring in the future.
> >>
> >> In the future, we will provide features similar to
> >> VHOST_USER_GET_INFLIGHT_FD
> >> so the device can save pending operations.
> >> ```
> >>
> >> Thanks for pointing it out!
> >>
> >>
> >>
> >>
> >>
> >>> E.g. do I assume correctly all in flight requests are flushed after
> >>> return from this uAPI call? Or some of pending requests may be subject
> >>> to loss or failure? How does the caller/user specify these various
> >>> options (if there are) for device stop?
> >>>
> >>> BTW, it would be nice to add the corresponding support to vdpa_sim_blk
> >>> as well to demo the stop handling. To just show it on vdpa-sim-net IMHO
> >>> is perhaps not so convincing.
> >>>
> >>> -Siwei
> >>>
> >>>>     * @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);
> >
>

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

[Eugenio Perez Martin]

On Tue, May 24, 2022 at 9:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Mon, May 23, 2022 at 09:20:14PM +0200, Eugenio Perez Martin wrote:
> >On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >>
> >>
> >>
> >> On 5/20/2022 10:23 AM, 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)
> >> > + *                           Returns integer: success (0) or error (< 0)
> >> Is this uAPI meant to address all use cases described in the full blown
> >> _F_STOP virtio spec proposal, such as:
> >>
> >> --------------%<--------------
> >>
> >> ...... the device MUST finish any in flight
> >> operations after the driver writes STOP.  Depending on the device, it
> >> can do it
> >> in many ways as long as the driver can recover its normal operation
> >> if it
> >> resumes the device without the need of resetting it:
> >>
> >> - Drain and wait for the completion of all pending requests until a
> >>    convenient avail descriptor. Ignore any other posterior descriptor.
> >> - Return a device-specific failure for these descriptors, so the driver
> >>    can choose to retry or to cancel them.
> >> - Mark them as done even if they are not, if the kind of device can
> >>    assume to lose them.
> >> --------------%<--------------
> >>
> >
> >Right, this is totally underspecified in this series.
> >
> >I'll expand on it in the next version, but that text proposed to
> >virtio-comment was complicated and misleading. I find better to get
> >the previous version description. Would the next description work?
> >
> >```
> >After the return of ioctl, 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.
>
> For block devices wait for all in-flight requests could take several
> time.
>
> Could this be a problem if the caller gets stuck on this ioctl?
>
> If it could be a problem, maybe we should use an eventfd to signal that
> the device is successfully stopped.
>

For that particular problem I'd very much prefer to add directly an
ioctl to get the inflight descriptors. We know for sure we will need
them, and it will be cleaner in the long run.

As I understand the vdpa block simulator, there is no need to return
the inflight descriptors since all of the requests are processed in a
synchronous way. So, for this iteration, we could offer the stop
feature to qemu.

Other non-simulated devices would need it. Could it be delayed to
future development?

Thanks!

> Thanks,
> Stefano
>

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

[Stefano Garzarella]

On Tue, May 24, 2022 at 09:42:06AM +0200, Eugenio Perez Martin wrote:
>On Tue, May 24, 2022 at 9:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Mon, May 23, 2022 at 09:20:14PM +0200, Eugenio Perez Martin wrote:
>> >On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>> >>
>> >>
>> >>
>> >> On 5/20/2022 10:23 AM, 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)
>> >> > + *                           Returns integer: success (0) or error (< 0)
>> >> Is this uAPI meant to address all use cases described in the full blown
>> >> _F_STOP virtio spec proposal, such as:
>> >>
>> >> --------------%<--------------
>> >>
>> >> ...... the device MUST finish any in flight
>> >> operations after the driver writes STOP.  Depending on the device, it
>> >> can do it
>> >> in many ways as long as the driver can recover its normal operation
>> >> if it
>> >> resumes the device without the need of resetting it:
>> >>
>> >> - Drain and wait for the completion of all pending requests until a
>> >>    convenient avail descriptor. Ignore any other posterior descriptor.
>> >> - Return a device-specific failure for these descriptors, so the driver
>> >>    can choose to retry or to cancel them.
>> >> - Mark them as done even if they are not, if the kind of device can
>> >>    assume to lose them.
>> >> --------------%<--------------
>> >>
>> >
>> >Right, this is totally underspecified in this series.
>> >
>> >I'll expand on it in the next version, but that text proposed to
>> >virtio-comment was complicated and misleading. I find better to get
>> >the previous version description. Would the next description work?
>> >
>> >```
>> >After the return of ioctl, 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.
>>
>> For block devices wait for all in-flight requests could take several
>> time.
>>
>> Could this be a problem if the caller gets stuck on this ioctl?
>>
>> If it could be a problem, maybe we should use an eventfd to signal that
>> the device is successfully stopped.
>>
>
>For that particular problem I'd very much prefer to add directly an
>ioctl to get the inflight descriptors. We know for sure we will need
>them, and it will be cleaner in the long run.

Makes sense!

>
>As I understand the vdpa block simulator, there is no need to return
>the inflight descriptors since all of the requests are processed in a
>synchronous way. So, for this iteration, we could offer the stop
>feature to qemu.

Right, the simulator handles everything synchronously.

>
>Other non-simulated devices would need it. Could it be delayed to
>future development?

Yep, sure, it sounds like you already have a plan, so no problem :-)

Thanks,
Stefano

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

[Stefano Garzarella]

On Tue, May 24, 2022 at 09:42:06AM +0200, Eugenio Perez Martin wrote:
>On Tue, May 24, 2022 at 9:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Mon, May 23, 2022 at 09:20:14PM +0200, Eugenio Perez Martin wrote:
>> >On Sat, May 21, 2022 at 12:13 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>> >>
>> >>
>> >>
>> >> On 5/20/2022 10:23 AM, 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)
>> >> > + *                           Returns integer: success (0) or error (< 0)
>> >> Is this uAPI meant to address all use cases described in the full blown
>> >> _F_STOP virtio spec proposal, such as:
>> >>
>> >> --------------%<--------------
>> >>
>> >> ...... the device MUST finish any in flight
>> >> operations after the driver writes STOP.  Depending on the device, it
>> >> can do it
>> >> in many ways as long as the driver can recover its normal operation
>> >> if it
>> >> resumes the device without the need of resetting it:
>> >>
>> >> - Drain and wait for the completion of all pending requests until a
>> >>    convenient avail descriptor. Ignore any other posterior descriptor.
>> >> - Return a device-specific failure for these descriptors, so the driver
>> >>    can choose to retry or to cancel them.
>> >> - Mark them as done even if they are not, if the kind of device can
>> >>    assume to lose them.
>> >> --------------%<--------------
>> >>
>> >
>> >Right, this is totally underspecified in this series.
>> >
>> >I'll expand on it in the next version, but that text proposed to
>> >virtio-comment was complicated and misleading. I find better to get
>> >the previous version description. Would the next description work?
>> >
>> >```
>> >After the return of ioctl, 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.
>>
>> For block devices wait for all in-flight requests could take several
>> time.
>>
>> Could this be a problem if the caller gets stuck on this ioctl?
>>
>> If it could be a problem, maybe we should use an eventfd to signal that
>> the device is successfully stopped.
>>
>
>For that particular problem I'd very much prefer to add directly an
>ioctl to get the inflight descriptors. We know for sure we will need
>them, and it will be cleaner in the long run.

Makes sense!

>
>As I understand the vdpa block simulator, there is no need to return
>the inflight descriptors since all of the requests are processed in a
>synchronous way. So, for this iteration, we could offer the stop
>feature to qemu.

Right, the simulator handles everything synchronously.

>
>Other non-simulated devices would need it. Could it be delayed to
>future development?

Yep, sure, it sounds like you already have a plan, so no problem :-)

Thanks,
Stefano

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