[PATCH v3 0/4] Implement vdpasim stop operation

[PATCH v3 0/4] Implement vdpasim stop operation

[Eugenio Pérez]

Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
that backend feature and userspace can effectively stop the device.

This is a must before get virtqueue indexes (base) for live migration,
since the device could modify them after userland gets them. There are
individual ways to perform that action for some devices
(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
way to perform it for any vhost device (and, in particular, vhost-vdpa).

After the return of ioctl with stop != 0, the device MUST finish any
pending operations like in flight requests. It must also preserve all
the necessary state (the virtqueue vring base plus the possible device
specific states) that is required for restoring in the future. The
device must not change its configuration after that point.

After the return of ioctl with stop == 0, the device can continue
processing buffers as long as typical conditions are met (vq is enabled,
DRIVER_OK status bit is enabled, etc).

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

Comments are welcome.

v3:
* s/VHOST_STOP/VHOST_VDPA_STOP/
* Add documentation and requirements of the ioctl above its definition.

v2:
* Replace raw _F_STOP with BIT_ULL(_F_STOP).
* Fix obtaining of stop ioctl arg (it was not obtained but written).
* Add stop to vdpa_sim_blk.

Eugenio Pérez (4):
  vdpa: Add stop operation
  vhost-vdpa: introduce STOP backend feature bit
  vhost-vdpa: uAPI to stop the device
  vdpa_sim: Implement stop vdpa op

 drivers/vdpa/vdpa_sim/vdpa_sim.c     | 21 +++++++++++++++++
 drivers/vdpa/vdpa_sim/vdpa_sim.h     |  1 +
 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c |  3 +++
 drivers/vdpa/vdpa_sim/vdpa_sim_net.c |  3 +++
 drivers/vhost/vdpa.c                 | 34 +++++++++++++++++++++++++++-
 include/linux/vdpa.h                 |  6 +++++
 include/uapi/linux/vhost.h           | 14 ++++++++++++
 include/uapi/linux/vhost_types.h     |  2 ++
 8 files changed, 83 insertions(+), 1 deletion(-)

-- 
2.27.0

[PATCH v3 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 v3 2/4] vhost-vdpa: introduce STOP backend feature bit

[Eugenio Pérez]

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

It's only offered if the vdpa driver backend implements the stop()
operation callback, and try to set it if the backend does not offer that
callback is an error.

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

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 1f1d1c425573..32713db5831d 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 	return 0;
 }
 
+static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v)
+{
+	struct vdpa_device *vdpa = v->vdpa;
+	const struct vdpa_config_ops *ops = vdpa->config;
+
+	return ops->stop;
+}
+
 static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
 {
 	struct vdpa_device *vdpa = v->vdpa;
@@ -575,7 +583,11 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
 	if (cmd == VHOST_SET_BACKEND_FEATURES) {
 		if (copy_from_user(&features, featurep, sizeof(features)))
 			return -EFAULT;
-		if (features & ~VHOST_VDPA_BACKEND_FEATURES)
+		if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
+				 BIT_ULL(VHOST_BACKEND_F_STOP)))
+			return -EOPNOTSUPP;
+		if ((features & BIT_ULL(VHOST_BACKEND_F_STOP)) &&
+		     !vhost_vdpa_can_stop(v))
 			return -EOPNOTSUPP;
 		vhost_set_backend_features(&v->vdev, features);
 		return 0;
@@ -624,6 +636,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
 		break;
 	case VHOST_GET_BACKEND_FEATURES:
 		features = VHOST_VDPA_BACKEND_FEATURES;
+		if (vhost_vdpa_can_stop(v))
+			features |= BIT_ULL(VHOST_BACKEND_F_STOP);
 		if (copy_to_user(featurep, &features, sizeof(features)))
 			r = -EFAULT;
 		break;
diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
index 634cee485abb..2758e665791b 100644
--- a/include/uapi/linux/vhost_types.h
+++ b/include/uapi/linux/vhost_types.h
@@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
  * message
  */
 #define VHOST_BACKEND_F_IOTLB_ASID  0x3
+/* Stop device from processing virtqueue buffers */
+#define VHOST_BACKEND_F_STOP  0x4
 
 #endif
-- 
2.27.0

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

[Eugenio Pérez]

The ioctl adds support for stop the device from userspace.

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

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 32713db5831d..a5d33bad92f9 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -478,6 +478,21 @@ static long vhost_vdpa_get_vqs_count(struct vhost_vdpa *v, u32 __user *argp)
 	return 0;
 }
 
+static long vhost_vdpa_stop(struct vhost_vdpa *v, u32 __user *argp)
+{
+	struct vdpa_device *vdpa = v->vdpa;
+	const struct vdpa_config_ops *ops = vdpa->config;
+	int stop;
+
+	if (!ops->stop)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&stop, argp, sizeof(stop)))
+		return -EFAULT;
+
+	return ops->stop(vdpa, stop);
+}
+
 static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
 				   void __user *argp)
 {
@@ -650,6 +665,9 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
 	case VHOST_VDPA_GET_VQS_COUNT:
 		r = vhost_vdpa_get_vqs_count(v, argp);
 		break;
+	case VHOST_STOP:
+		r = vhost_vdpa_stop(v, argp);
+		break;
 	default:
 		r = vhost_dev_ioctl(&v->vdev, cmd, argp);
 		if (r == -ENOIOCTLCMD)
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index cab645d4a645..c7e47b29bf61 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -171,4 +171,18 @@
 #define VHOST_VDPA_SET_GROUP_ASID	_IOW(VHOST_VIRTIO, 0x7C, \
 					     struct vhost_vring_state)
 
+/* Stop or resume a device so it does not process virtqueue requests anymore
+ *
+ * After the return of ioctl with stop != 0, the device must finish any
+ * pending operations like in flight requests. It must also preserve all
+ * the necessary state (the virtqueue vring base plus the possible device
+ * specific states) that is required for restoring in the future. The
+ * device must not change its configuration after that point.
+ *
+ * After the return of ioctl with stop == 0, the device can continue
+ * processing buffers as long as typical conditions are met (vq is enabled,
+ * DRIVER_OK status bit is enabled, etc).
+ */
+#define VHOST_VDPA_STOP			_IOW(VHOST_VIRTIO, 0x7D, int)
+
 #endif
-- 
2.27.0

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

[Eugenio Pérez]

Implement stop operation for vdpa_sim devices, so vhost-vdpa will offer
that backend feature and userspace can effectively stop the device.

This is a must before get virtqueue indexes (base) for live migration,
since the device could modify them after userland gets them. There are
individual ways to perform that action for some devices
(VHOST_NET_SET_BACKEND, VHOST_VSOCK_SET_RUNNING, ...) but there was no
way to perform it for any vhost device (and, in particular, vhost-vdpa).

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim.c     | 21 +++++++++++++++++++++
 drivers/vdpa/vdpa_sim/vdpa_sim.h     |  1 +
 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c |  3 +++
 drivers/vdpa/vdpa_sim/vdpa_sim_net.c |  3 +++
 4 files changed, 28 insertions(+)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 50d721072beb..0515cf314bed 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -107,6 +107,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
 	for (i = 0; i < vdpasim->dev_attr.nas; i++)
 		vhost_iotlb_reset(&vdpasim->iommu[i]);
 
+	vdpasim->running = true;
 	spin_unlock(&vdpasim->iommu_lock);
 
 	vdpasim->features = 0;
@@ -505,6 +506,24 @@ static int vdpasim_reset(struct vdpa_device *vdpa)
 	return 0;
 }
 
+static int vdpasim_stop(struct vdpa_device *vdpa, bool stop)
+{
+	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+	int i;
+
+	spin_lock(&vdpasim->lock);
+	vdpasim->running = !stop;
+	if (vdpasim->running) {
+		/* Check for missed buffers */
+		for (i = 0; i < vdpasim->dev_attr.nvqs; ++i)
+			vdpasim_kick_vq(vdpa, i);
+
+	}
+	spin_unlock(&vdpasim->lock);
+
+	return 0;
+}
+
 static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
 {
 	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
@@ -694,6 +713,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
 	.get_status             = vdpasim_get_status,
 	.set_status             = vdpasim_set_status,
 	.reset			= vdpasim_reset,
+	.stop			= vdpasim_stop,
 	.get_config_size        = vdpasim_get_config_size,
 	.get_config             = vdpasim_get_config,
 	.set_config             = vdpasim_set_config,
@@ -726,6 +746,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
 	.get_status             = vdpasim_get_status,
 	.set_status             = vdpasim_set_status,
 	.reset			= vdpasim_reset,
+	.stop			= vdpasim_stop,
 	.get_config_size        = vdpasim_get_config_size,
 	.get_config             = vdpasim_get_config,
 	.set_config             = vdpasim_set_config,
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
index 622782e92239..061986f30911 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
@@ -66,6 +66,7 @@ struct vdpasim {
 	u32 generation;
 	u64 features;
 	u32 groups;
+	bool running;
 	/* spinlock to synchronize iommu table */
 	spinlock_t iommu_lock;
 };
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index 42d401d43911..bcdb1982c378 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -204,6 +204,9 @@ static void vdpasim_blk_work(struct work_struct *work)
 	if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
 		goto out;
 
+	if (!vdpasim->running)
+		goto out;
+
 	for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
 		struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
 
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
index 5125976a4df8..886449e88502 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -154,6 +154,9 @@ static void vdpasim_net_work(struct work_struct *work)
 
 	spin_lock(&vdpasim->lock);
 
+	if (!vdpasim->running)
+		goto out;
+
 	if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
 		goto out;
 
-- 
2.27.0

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

[Dawar, Gautam]

[AMD Official Use Only - General]

-----Original Message-----
From: Eugenio Pérez <eperezma@redhat.com>
Sent: Wednesday, May 25, 2022 4:29 PM
To: Michael S. Tsirkin <mst@redhat.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org; virtualization@lists.linux-foundation.org; Jason Wang <jasowang@redhat.com>
Cc: Zhu Lingshan <lingshan.zhu@intel.com>; martinh@xilinx.com; Stefano Garzarella <sgarzare@redhat.com>; ecree.xilinx@gmail.com; Eli Cohen <elic@nvidia.com>; Dan Carpenter <dan.carpenter@oracle.com>; Parav Pandit <parav@nvidia.com>; Wu Zongyong <wuzongyong@linux.alibaba.com>; dinang@xilinx.com; Christophe JAILLET <christophe.jaillet@wanadoo.fr>; Xie Yongji <xieyongji@bytedance.com>; Dawar, Gautam <gautam.dawar@amd.com>; lulu@redhat.com; martinpo@xilinx.com; pabloc@xilinx.com; Longpeng <longpeng2@huawei.com>; Piotr.Uminski@intel.com; Kamde, Tanuj <tanuj.kamde@amd.com>; Si-Wei Liu <si-wei.liu@oracle.com>; habetsm.xilinx@gmail.com; lvivier@redhat.com; Zhang Min <zhang.min9@zte.com.cn>; hanand@xilinx.com
Subject: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit

[CAUTION: External Email]

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

It's only offered if the vdpa driver backend implements the stop() operation callback, and try to set it if the backend does not offer that callback is an error.

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

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 1f1d1c425573..32713db5831d 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
        return 0;
 }

+static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v) {
+       struct vdpa_device *vdpa = v->vdpa;
+       const struct vdpa_config_ops *ops = vdpa->config;
+
+       return ops->stop;
[GD>>] Would it be better to explicitly return a bool to match the return type?
+}
+
 static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)  {
        struct vdpa_device *vdpa = v->vdpa; @@ -575,7 +583,11 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
        if (cmd == VHOST_SET_BACKEND_FEATURES) {
                if (copy_from_user(&features, featurep, sizeof(features)))
                        return -EFAULT;
-               if (features & ~VHOST_VDPA_BACKEND_FEATURES)
+               if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
+                                BIT_ULL(VHOST_BACKEND_F_STOP)))
+                       return -EOPNOTSUPP;
+               if ((features & BIT_ULL(VHOST_BACKEND_F_STOP)) &&
+                    !vhost_vdpa_can_stop(v))
                        return -EOPNOTSUPP;
                vhost_set_backend_features(&v->vdev, features);
                return 0;
@@ -624,6 +636,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
                break;
        case VHOST_GET_BACKEND_FEATURES:
                features = VHOST_VDPA_BACKEND_FEATURES;
+               if (vhost_vdpa_can_stop(v))
+                       features |= BIT_ULL(VHOST_BACKEND_F_STOP);
                if (copy_to_user(featurep, &features, sizeof(features)))
                        r = -EFAULT;
                break;
diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
index 634cee485abb..2758e665791b 100644
--- a/include/uapi/linux/vhost_types.h
+++ b/include/uapi/linux/vhost_types.h
@@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
  * message
  */
 #define VHOST_BACKEND_F_IOTLB_ASID  0x3
+/* Stop device from processing virtqueue buffers */ #define
+VHOST_BACKEND_F_STOP  0x4

 #endif
--
2.27.0

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

[kernel test robot]

Hi "Eugenio,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mst-vhost/linux-next]
[also build test ERROR on next-20220525]
[cannot apply to horms-ipvs/master linux/master linus/master v5.18]
[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/20220525-190143
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: x86_64-randconfig-a013 (https://download.01.org/0day-ci/archive/20220525/202205252236.4ysv1ZWg-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/515f6b6d2a0164df801ddbe61e1cb1ae4e763873
        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/20220525-190143
        git checkout 515f6b6d2a0164df801ddbe61e1cb1ae4e763873
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

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

All errors (new ones prefixed by >>):

   drivers/vhost/vdpa.c: In function 'vhost_vdpa_unlocked_ioctl':
>> drivers/vhost/vdpa.c:668:14: error: 'VHOST_STOP' undeclared (first use in this function)
     668 |         case VHOST_STOP:
         |              ^~~~~~~~~~
   drivers/vhost/vdpa.c:668:14: note: each undeclared identifier is reported only once for each function it appears in


vim +/VHOST_STOP +668 drivers/vhost/vdpa.c

   587	
   588	static long vhost_vdpa_unlocked_ioctl(struct file *filep,
   589					      unsigned int cmd, unsigned long arg)
   590	{
   591		struct vhost_vdpa *v = filep->private_data;
   592		struct vhost_dev *d = &v->vdev;
   593		void __user *argp = (void __user *)arg;
   594		u64 __user *featurep = argp;
   595		u64 features;
   596		long r = 0;
   597	
   598		if (cmd == VHOST_SET_BACKEND_FEATURES) {
   599			if (copy_from_user(&features, featurep, sizeof(features)))
   600				return -EFAULT;
   601			if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
   602					 BIT_ULL(VHOST_BACKEND_F_STOP)))
   603				return -EOPNOTSUPP;
   604			if ((features & BIT_ULL(VHOST_BACKEND_F_STOP)) &&
   605			     !vhost_vdpa_can_stop(v))
   606				return -EOPNOTSUPP;
   607			vhost_set_backend_features(&v->vdev, features);
   608			return 0;
   609		}
   610	
   611		mutex_lock(&d->mutex);
   612	
   613		switch (cmd) {
   614		case VHOST_VDPA_GET_DEVICE_ID:
   615			r = vhost_vdpa_get_device_id(v, argp);
   616			break;
   617		case VHOST_VDPA_GET_STATUS:
   618			r = vhost_vdpa_get_status(v, argp);
   619			break;
   620		case VHOST_VDPA_SET_STATUS:
   621			r = vhost_vdpa_set_status(v, argp);
   622			break;
   623		case VHOST_VDPA_GET_CONFIG:
   624			r = vhost_vdpa_get_config(v, argp);
   625			break;
   626		case VHOST_VDPA_SET_CONFIG:
   627			r = vhost_vdpa_set_config(v, argp);
   628			break;
   629		case VHOST_GET_FEATURES:
   630			r = vhost_vdpa_get_features(v, argp);
   631			break;
   632		case VHOST_SET_FEATURES:
   633			r = vhost_vdpa_set_features(v, argp);
   634			break;
   635		case VHOST_VDPA_GET_VRING_NUM:
   636			r = vhost_vdpa_get_vring_num(v, argp);
   637			break;
   638		case VHOST_VDPA_GET_GROUP_NUM:
   639			r = copy_to_user(argp, &v->vdpa->ngroups,
   640					 sizeof(v->vdpa->ngroups));
   641			break;
   642		case VHOST_VDPA_GET_AS_NUM:
   643			r = copy_to_user(argp, &v->vdpa->nas, sizeof(v->vdpa->nas));
   644			break;
   645		case VHOST_SET_LOG_BASE:
   646		case VHOST_SET_LOG_FD:
   647			r = -ENOIOCTLCMD;
   648			break;
   649		case VHOST_VDPA_SET_CONFIG_CALL:
   650			r = vhost_vdpa_set_config_call(v, argp);
   651			break;
   652		case VHOST_GET_BACKEND_FEATURES:
   653			features = VHOST_VDPA_BACKEND_FEATURES;
   654			if (vhost_vdpa_can_stop(v))
   655				features |= BIT_ULL(VHOST_BACKEND_F_STOP);
   656			if (copy_to_user(featurep, &features, sizeof(features)))
   657				r = -EFAULT;
   658			break;
   659		case VHOST_VDPA_GET_IOVA_RANGE:
   660			r = vhost_vdpa_get_iova_range(v, argp);
   661			break;
   662		case VHOST_VDPA_GET_CONFIG_SIZE:
   663			r = vhost_vdpa_get_config_size(v, argp);
   664			break;
   665		case VHOST_VDPA_GET_VQS_COUNT:
   666			r = vhost_vdpa_get_vqs_count(v, argp);
   667			break;
 > 668		case VHOST_STOP:
   669			r = vhost_vdpa_stop(v, argp);
   670			break;
   671		default:
   672			r = vhost_dev_ioctl(&v->vdev, cmd, argp);
   673			if (r == -ENOIOCTLCMD)
   674				r = vhost_vdpa_vring_ioctl(v, cmd, argp);
   675			break;
   676		}
   677	
   678		mutex_unlock(&d->mutex);
   679		return r;
   680	}
   681	

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

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

[kernel test robot]

Hi "Eugenio,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mst-vhost/linux-next]
[also build test ERROR on next-20220525]
[cannot apply to horms-ipvs/master linux/master linus/master v5.18]
[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/20220525-190143
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220526/202205260121.6V500tTl-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d52a6e75b0c402c7f3b42a2b1b2873f151220947)
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/515f6b6d2a0164df801ddbe61e1cb1ae4e763873
        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/20220525-190143
        git checkout 515f6b6d2a0164df801ddbe61e1cb1ae4e763873
        # 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=x86_64 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 errors (new ones prefixed by >>):

>> drivers/vhost/vdpa.c:668:7: error: use of undeclared identifier 'VHOST_STOP'
           case VHOST_STOP:
                ^
   1 error generated.


vim +/VHOST_STOP +668 drivers/vhost/vdpa.c

   587	
   588	static long vhost_vdpa_unlocked_ioctl(struct file *filep,
   589					      unsigned int cmd, unsigned long arg)
   590	{
   591		struct vhost_vdpa *v = filep->private_data;
   592		struct vhost_dev *d = &v->vdev;
   593		void __user *argp = (void __user *)arg;
   594		u64 __user *featurep = argp;
   595		u64 features;
   596		long r = 0;
   597	
   598		if (cmd == VHOST_SET_BACKEND_FEATURES) {
   599			if (copy_from_user(&features, featurep, sizeof(features)))
   600				return -EFAULT;
   601			if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
   602					 BIT_ULL(VHOST_BACKEND_F_STOP)))
   603				return -EOPNOTSUPP;
   604			if ((features & BIT_ULL(VHOST_BACKEND_F_STOP)) &&
   605			     !vhost_vdpa_can_stop(v))
   606				return -EOPNOTSUPP;
   607			vhost_set_backend_features(&v->vdev, features);
   608			return 0;
   609		}
   610	
   611		mutex_lock(&d->mutex);
   612	
   613		switch (cmd) {
   614		case VHOST_VDPA_GET_DEVICE_ID:
   615			r = vhost_vdpa_get_device_id(v, argp);
   616			break;
   617		case VHOST_VDPA_GET_STATUS:
   618			r = vhost_vdpa_get_status(v, argp);
   619			break;
   620		case VHOST_VDPA_SET_STATUS:
   621			r = vhost_vdpa_set_status(v, argp);
   622			break;
   623		case VHOST_VDPA_GET_CONFIG:
   624			r = vhost_vdpa_get_config(v, argp);
   625			break;
   626		case VHOST_VDPA_SET_CONFIG:
   627			r = vhost_vdpa_set_config(v, argp);
   628			break;
   629		case VHOST_GET_FEATURES:
   630			r = vhost_vdpa_get_features(v, argp);
   631			break;
   632		case VHOST_SET_FEATURES:
   633			r = vhost_vdpa_set_features(v, argp);
   634			break;
   635		case VHOST_VDPA_GET_VRING_NUM:
   636			r = vhost_vdpa_get_vring_num(v, argp);
   637			break;
   638		case VHOST_VDPA_GET_GROUP_NUM:
   639			r = copy_to_user(argp, &v->vdpa->ngroups,
   640					 sizeof(v->vdpa->ngroups));
   641			break;
   642		case VHOST_VDPA_GET_AS_NUM:
   643			r = copy_to_user(argp, &v->vdpa->nas, sizeof(v->vdpa->nas));
   644			break;
   645		case VHOST_SET_LOG_BASE:
   646		case VHOST_SET_LOG_FD:
   647			r = -ENOIOCTLCMD;
   648			break;
   649		case VHOST_VDPA_SET_CONFIG_CALL:
   650			r = vhost_vdpa_set_config_call(v, argp);
   651			break;
   652		case VHOST_GET_BACKEND_FEATURES:
   653			features = VHOST_VDPA_BACKEND_FEATURES;
   654			if (vhost_vdpa_can_stop(v))
   655				features |= BIT_ULL(VHOST_BACKEND_F_STOP);
   656			if (copy_to_user(featurep, &features, sizeof(features)))
   657				r = -EFAULT;
   658			break;
   659		case VHOST_VDPA_GET_IOVA_RANGE:
   660			r = vhost_vdpa_get_iova_range(v, argp);
   661			break;
   662		case VHOST_VDPA_GET_CONFIG_SIZE:
   663			r = vhost_vdpa_get_config_size(v, argp);
   664			break;
   665		case VHOST_VDPA_GET_VQS_COUNT:
   666			r = vhost_vdpa_get_vqs_count(v, argp);
   667			break;
 > 668		case VHOST_STOP:
   669			r = vhost_vdpa_stop(v, argp);
   670			break;
   671		default:
   672			r = vhost_dev_ioctl(&v->vdev, cmd, argp);
   673			if (r == -ENOIOCTLCMD)
   674				r = vhost_vdpa_vring_ioctl(v, cmd, argp);
   675			break;
   676		}
   677	
   678		mutex_unlock(&d->mutex);
   679		return r;
   680	}
   681	

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

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

[Eugenio Perez Martin]

On Wed, May 25, 2022 at 1:23 PM Dawar, Gautam <gautam.dawar@amd.com> wrote:
>
> [AMD Official Use Only - General]
>
> -----Original Message-----
> From: Eugenio Pérez <eperezma@redhat.com>
> Sent: Wednesday, May 25, 2022 4:29 PM
> To: Michael S. Tsirkin <mst@redhat.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org; virtualization@lists.linux-foundation.org; Jason Wang <jasowang@redhat.com>
> Cc: Zhu Lingshan <lingshan.zhu@intel.com>; martinh@xilinx.com; Stefano Garzarella <sgarzare@redhat.com>; ecree.xilinx@gmail.com; Eli Cohen <elic@nvidia.com>; Dan Carpenter <dan.carpenter@oracle.com>; Parav Pandit <parav@nvidia.com>; Wu Zongyong <wuzongyong@linux.alibaba.com>; dinang@xilinx.com; Christophe JAILLET <christophe.jaillet@wanadoo.fr>; Xie Yongji <xieyongji@bytedance.com>; Dawar, Gautam <gautam.dawar@amd.com>; lulu@redhat.com; martinpo@xilinx.com; pabloc@xilinx.com; Longpeng <longpeng2@huawei.com>; Piotr.Uminski@intel.com; Kamde, Tanuj <tanuj.kamde@amd.com>; Si-Wei Liu <si-wei.liu@oracle.com>; habetsm.xilinx@gmail.com; lvivier@redhat.com; Zhang Min <zhang.min9@zte.com.cn>; hanand@xilinx.com
> Subject: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit
>
> [CAUTION: External Email]
>
> Userland knows if it can stop the device or not by checking this feature bit.
>
> It's only offered if the vdpa driver backend implements the stop() operation callback, and try to set it if the backend does not offer that callback is an error.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  drivers/vhost/vdpa.c             | 16 +++++++++++++++-
>  include/uapi/linux/vhost_types.h |  2 ++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 1f1d1c425573..32713db5831d 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
>         return 0;
>  }
>
> +static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v) {
> +       struct vdpa_device *vdpa = v->vdpa;
> +       const struct vdpa_config_ops *ops = vdpa->config;
> +
> +       return ops->stop;
> [GD>>] Would it be better to explicitly return a bool to match the return type?

I'm not sure about the kernel code style regarding that casting. Maybe
it's better to return !!ops->stop here. The macros likely and unlikely
do that.

Thanks!

> +}
> +
>  static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)  {
>         struct vdpa_device *vdpa = v->vdpa; @@ -575,7 +583,11 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
>         if (cmd == VHOST_SET_BACKEND_FEATURES) {
>                 if (copy_from_user(&features, featurep, sizeof(features)))
>                         return -EFAULT;
> -               if (features & ~VHOST_VDPA_BACKEND_FEATURES)
> +               if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
> +                                BIT_ULL(VHOST_BACKEND_F_STOP)))
> +                       return -EOPNOTSUPP;
> +               if ((features & BIT_ULL(VHOST_BACKEND_F_STOP)) &&
> +                    !vhost_vdpa_can_stop(v))
>                         return -EOPNOTSUPP;
>                 vhost_set_backend_features(&v->vdev, features);
>                 return 0;
> @@ -624,6 +636,8 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
>                 break;
>         case VHOST_GET_BACKEND_FEATURES:
>                 features = VHOST_VDPA_BACKEND_FEATURES;
> +               if (vhost_vdpa_can_stop(v))
> +                       features |= BIT_ULL(VHOST_BACKEND_F_STOP);
>                 if (copy_to_user(featurep, &features, sizeof(features)))
>                         r = -EFAULT;
>                 break;
> diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
> index 634cee485abb..2758e665791b 100644
> --- a/include/uapi/linux/vhost_types.h
> +++ b/include/uapi/linux/vhost_types.h
> @@ -161,5 +161,7 @@ struct vhost_vdpa_iova_range {
>   * message
>   */
>  #define VHOST_BACKEND_F_IOTLB_ASID  0x3
> +/* Stop device from processing virtqueue buffers */ #define
> +VHOST_BACKEND_F_STOP  0x4
>
>  #endif
> --
> 2.27.0
>

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

[Stefano Garzarella]

On Thu, May 26, 2022 at 10:57:03AM +0200, Eugenio Perez Martin wrote:
>On Wed, May 25, 2022 at 1:23 PM Dawar, Gautam <gautam.dawar@amd.com> wrote:
>>
>> [AMD Official Use Only - General]
>>
>> -----Original Message-----
>> From: Eugenio Pérez <eperezma@redhat.com>
>> Sent: Wednesday, May 25, 2022 4:29 PM
>> To: Michael S. Tsirkin <mst@redhat.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org; virtualization@lists.linux-foundation.org; Jason Wang <jasowang@redhat.com>
>> Cc: Zhu Lingshan <lingshan.zhu@intel.com>; martinh@xilinx.com; Stefano Garzarella <sgarzare@redhat.com>; ecree.xilinx@gmail.com; Eli Cohen <elic@nvidia.com>; Dan Carpenter <dan.carpenter@oracle.com>; Parav Pandit <parav@nvidia.com>; Wu Zongyong <wuzongyong@linux.alibaba.com>; dinang@xilinx.com; Christophe JAILLET <christophe.jaillet@wanadoo.fr>; Xie Yongji <xieyongji@bytedance.com>; Dawar, Gautam <gautam.dawar@amd.com>; lulu@redhat.com; martinpo@xilinx.com; pabloc@xilinx.com; Longpeng <longpeng2@huawei.com>; Piotr.Uminski@intel.com; Kamde, Tanuj <tanuj.kamde@amd.com>; Si-Wei Liu <si-wei.liu@oracle.com>; habetsm.xilinx@gmail.com; lvivier@redhat.com; Zhang Min <zhang.min9@zte.com.cn>; hanand@xilinx.com
>> Subject: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit
>>
>> [CAUTION: External Email]
>>
>> Userland knows if it can stop the device or not by checking this feature bit.
>>
>> It's only offered if the vdpa driver backend implements the stop() operation callback, and try to set it if the backend does not offer that callback is an error.
>>
>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> ---
>>  drivers/vhost/vdpa.c             | 16 +++++++++++++++-
>>  include/uapi/linux/vhost_types.h |  2 ++
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 1f1d1c425573..32713db5831d 100644
>> --- a/drivers/vhost/vdpa.c
>> +++ b/drivers/vhost/vdpa.c
>> @@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
>>         return 0;
>>  }
>>
>> +static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v) {
>> +       struct vdpa_device *vdpa = v->vdpa;
>> +       const struct vdpa_config_ops *ops = vdpa->config;
>> +
>> +       return ops->stop;
>> [GD>>] Would it be better to explicitly return a bool to match the return type?
>
>I'm not sure about the kernel code style regarding that casting. Maybe
>it's better to return !!ops->stop here. The macros likely and unlikely
>do that.

IIUC `ops->stop` is a function pointer, so what about

     return ops->stop != NULL;

Thanks,
Stefano

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

[Eugenio Perez Martin]

On Thu, May 26, 2022 at 11:07 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, May 26, 2022 at 10:57:03AM +0200, Eugenio Perez Martin wrote:
> >On Wed, May 25, 2022 at 1:23 PM Dawar, Gautam <gautam.dawar@amd.com> wrote:
> >>
> >> [AMD Official Use Only - General]
> >>
> >> -----Original Message-----
> >> From: Eugenio Pérez <eperezma@redhat.com>
> >> Sent: Wednesday, May 25, 2022 4:29 PM
> >> To: Michael S. Tsirkin <mst@redhat.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org; virtualization@lists.linux-foundation.org; Jason Wang <jasowang@redhat.com>
> >> Cc: Zhu Lingshan <lingshan.zhu@intel.com>; martinh@xilinx.com; Stefano Garzarella <sgarzare@redhat.com>; ecree.xilinx@gmail.com; Eli Cohen <elic@nvidia.com>; Dan Carpenter <dan.carpenter@oracle.com>; Parav Pandit <parav@nvidia.com>; Wu Zongyong <wuzongyong@linux.alibaba.com>; dinang@xilinx.com; Christophe JAILLET <christophe.jaillet@wanadoo.fr>; Xie Yongji <xieyongji@bytedance.com>; Dawar, Gautam <gautam.dawar@amd.com>; lulu@redhat.com; martinpo@xilinx.com; pabloc@xilinx.com; Longpeng <longpeng2@huawei.com>; Piotr.Uminski@intel.com; Kamde, Tanuj <tanuj.kamde@amd.com>; Si-Wei Liu <si-wei.liu@oracle.com>; habetsm.xilinx@gmail.com; lvivier@redhat.com; Zhang Min <zhang.min9@zte.com.cn>; hanand@xilinx.com
> >> Subject: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit
> >>
> >> [CAUTION: External Email]
> >>
> >> Userland knows if it can stop the device or not by checking this feature bit.
> >>
> >> It's only offered if the vdpa driver backend implements the stop() operation callback, and try to set it if the backend does not offer that callback is an error.
> >>
> >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >> ---
> >>  drivers/vhost/vdpa.c             | 16 +++++++++++++++-
> >>  include/uapi/linux/vhost_types.h |  2 ++
> >>  2 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 1f1d1c425573..32713db5831d 100644
> >> --- a/drivers/vhost/vdpa.c
> >> +++ b/drivers/vhost/vdpa.c
> >> @@ -347,6 +347,14 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
> >>         return 0;
> >>  }
> >>
> >> +static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v) {
> >> +       struct vdpa_device *vdpa = v->vdpa;
> >> +       const struct vdpa_config_ops *ops = vdpa->config;
> >> +
> >> +       return ops->stop;
> >> [GD>>] Would it be better to explicitly return a bool to match the return type?
> >
> >I'm not sure about the kernel code style regarding that casting. Maybe
> >it's better to return !!ops->stop here. The macros likely and unlikely
> >do that.
>
> IIUC `ops->stop` is a function pointer, so what about
>
>      return ops->stop != NULL;
>

I'm ok with any method proposed. Both three ways can be found in the
kernel so I think they are all valid (although the double negation is
from bool to integer in (0,1) set actually).

Maybe Jason or Michael (as maintainers) can state the preferred method here.

Generally I prefer explicit conversions, both signed and from/to
different types length. But I find conversion to bool to be simple
enough to be an exception to the rule. Same with void *. Let's see!

Sending v4 without this changed, waiting for answers.

Thanks!

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

[Dan Carpenter]

On Thu, May 26, 2022 at 02:44:02PM +0200, Eugenio Perez Martin wrote:
> > >> +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;
> > >> [GD>>] Would it be better to explicitly return a bool to match the return type?
> > >
> > >I'm not sure about the kernel code style regarding that casting. Maybe
> > >it's better to return !!ops->stop here. The macros likely and unlikely
> > >do that.
> >
> > IIUC `ops->stop` is a function pointer, so what about
> >
> >      return ops->stop != NULL;
> >
> 
> I'm ok with any method proposed. Both three ways can be found in the
> kernel so I think they are all valid (although the double negation is
> from bool to integer in (0,1) set actually).
> 
> Maybe Jason or Michael (as maintainers) can state the preferred method here.

Always just do whatever the person who responded feels like because
they're likely the person who cares the most.  ;)

I don't think there are any static analysis tools which will complain
about this.  Smatch will complain if you return a negative literal.
It feels like returning any literal that isn't 1 or 0 should trigger a
warning...  I've written that and will check it out tonight.

Really anything negative should trigger a warning.  See new Smatch stuff
below.

regards,
dan carpenter

================ TEST CASE =========================

int x;
_Bool one(int *p)
{
        if (p)
                x = -2;
        return x;
}
_Bool two(int *p)
{
        return -4;  // returning 2 triggers a warning now
}

=============== OUTPUT =============================

test.c:10 one() warn: potential negative cast to bool 'x'
test.c:14 two() warn: signedness bug returning '(-4)'
test.c:14 two() warn: '(-4)' is not bool

=============== CODE ===============================

#include "smatch.h"
#include "smatch_extra.h"
#include "smatch_slist.h"

static int my_id;

static void match_literals(struct expression *ret_value)
{
	struct symbol *type;
	sval_t sval;

	type = cur_func_return_type();
	if (!type || sval_type_max(type).value != 1)
		return;

	if (!get_implied_value(ret_value, &sval))
		return;

	if (sval.value == 0 || sval.value == 1)
		return;

	sm_warning("'%s' is not bool", sval_to_str(sval));
}

static void match_any_negative(struct expression *ret_value)
{
	struct symbol *type;
	struct sm_state *extra, *sm;
	sval_t sval;
	char *name;

	type = cur_func_return_type();
	if (!type || sval_type_max(type).value != 1)
		return;

	extra = get_extra_sm_state(ret_value);
	if (!extra)
		return;
	FOR_EACH_PTR(extra->possible, sm) {
		if (estate_get_single_value(sm->state, &sval) &&
		    sval_is_negative(sval)) {
			name = expr_to_str(ret_value);
			sm_warning("potential negative cast to bool '%s'", name);
			free_string(name);
			return;
		}
	} END_FOR_EACH_PTR(sm);
}

void check_bool_return(int id)
{
	my_id = id;

	add_hook(&match_literals, RETURN_HOOK);
	add_hook(&match_any_negative, RETURN_HOOK);
}

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

[Eugenio Perez Martin]

On Thu, May 26, 2022 at 3:21 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, May 26, 2022 at 02:44:02PM +0200, Eugenio Perez Martin wrote:
> > > >> +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;
> > > >> [GD>>] Would it be better to explicitly return a bool to match the return type?
> > > >
> > > >I'm not sure about the kernel code style regarding that casting. Maybe
> > > >it's better to return !!ops->stop here. The macros likely and unlikely
> > > >do that.
> > >
> > > IIUC `ops->stop` is a function pointer, so what about
> > >
> > >      return ops->stop != NULL;
> > >
> >
> > I'm ok with any method proposed. Both three ways can be found in the
> > kernel so I think they are all valid (although the double negation is
> > from bool to integer in (0,1) set actually).
> >
> > Maybe Jason or Michael (as maintainers) can state the preferred method here.
>
> Always just do whatever the person who responded feels like because
> they're likely the person who cares the most.  ;)
>

This is interesting and I think it's good advice :). I'm fine with
whatever we chose, I just wanted to "break the tie" between the three.

> I don't think there are any static analysis tools which will complain
> about this.  Smatch will complain if you return a negative literal.

Maybe a negative literal is a bad code signal, yes.

> It feels like returning any literal that isn't 1 or 0 should trigger a
> warning...  I've written that and will check it out tonight.
>

I'm not sure this should be so strict, or "literal" does not include pointers?

As an experiment, can Smatch be used to count how many times a
returned pointer is converted to int / bool before returning vs not
converted?

I find Smatch interesting, especially when switching between projects
frequently. Does it support changing the code like clang-format? To
offload cognitive load to tools is usually good :).

Thanks!

> Really anything negative should trigger a warning.  See new Smatch stuff
> below.
>
> regards,
> dan carpenter
>
> ================ TEST CASE =========================
>
> int x;
> _Bool one(int *p)
> {
>         if (p)
>                 x = -2;
>         return x;
> }
> _Bool two(int *p)
> {
>         return -4;  // returning 2 triggers a warning now
> }
>
> =============== OUTPUT =============================
>
> test.c:10 one() warn: potential negative cast to bool 'x'
> test.c:14 two() warn: signedness bug returning '(-4)'
> test.c:14 two() warn: '(-4)' is not bool
>
> =============== CODE ===============================
>
> #include "smatch.h"
> #include "smatch_extra.h"
> #include "smatch_slist.h"
>
> static int my_id;
>
> static void match_literals(struct expression *ret_value)
> {
>         struct symbol *type;
>         sval_t sval;
>
>         type = cur_func_return_type();
>         if (!type || sval_type_max(type).value != 1)
>                 return;
>
>         if (!get_implied_value(ret_value, &sval))
>                 return;
>
>         if (sval.value == 0 || sval.value == 1)
>                 return;
>
>         sm_warning("'%s' is not bool", sval_to_str(sval));
> }
>
> static void match_any_negative(struct expression *ret_value)
> {
>         struct symbol *type;
>         struct sm_state *extra, *sm;
>         sval_t sval;
>         char *name;
>
>         type = cur_func_return_type();
>         if (!type || sval_type_max(type).value != 1)
>                 return;
>
>         extra = get_extra_sm_state(ret_value);
>         if (!extra)
>                 return;
>         FOR_EACH_PTR(extra->possible, sm) {
>                 if (estate_get_single_value(sm->state, &sval) &&
>                     sval_is_negative(sval)) {
>                         name = expr_to_str(ret_value);
>                         sm_warning("potential negative cast to bool '%s'", name);
>                         free_string(name);
>                         return;
>                 }
>         } END_FOR_EACH_PTR(sm);
> }
>
> void check_bool_return(int id)
> {
>         my_id = id;
>
>         add_hook(&match_literals, RETURN_HOOK);
>         add_hook(&match_any_negative, RETURN_HOOK);
> }
>

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

[Dan Carpenter]

On Thu, May 26, 2022 at 07:00:06PM +0200, Eugenio Perez Martin wrote:
> > It feels like returning any literal that isn't 1 or 0 should trigger a
> > warning...  I've written that and will check it out tonight.
> >
> 
> I'm not sure this should be so strict, or "literal" does not include pointers?
> 

What I mean in exact terms, is that if you're returning a known value
and the function returns bool then the known value should be 0 or 1.
Don't "return 3;".  This new warning will complain if you return a known
pointer as in "return &a;".  It won't complain if you return an
unknown pointer "return p;".

> As an experiment, can Smatch be used to count how many times a
> returned pointer is converted to int / bool before returning vs not
> converted?

I'm not super excited to write that code...  :/

> 
> I find Smatch interesting, especially when switching between projects
> frequently. Does it support changing the code like clang-format? To
> offload cognitive load to tools is usually good :).

No.  Coccinelle does that really well though.

regards,
dan carpenter

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

[Eugenio Perez Martin]

On Thu, May 26, 2022 at 9:07 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, May 26, 2022 at 07:00:06PM +0200, Eugenio Perez Martin wrote:
> > > It feels like returning any literal that isn't 1 or 0 should trigger a
> > > warning...  I've written that and will check it out tonight.
> > >
> >
> > I'm not sure this should be so strict, or "literal" does not include pointers?
> >
>
> What I mean in exact terms, is that if you're returning a known value
> and the function returns bool then the known value should be 0 or 1.
> Don't "return 3;".  This new warning will complain if you return a known
> pointer as in "return &a;".  It won't complain if you return an
> unknown pointer "return p;".
>

Ok, thanks for the clarification.

> > As an experiment, can Smatch be used to count how many times a
> > returned pointer is converted to int / bool before returning vs not
> > converted?
>
> I'm not super excited to write that code...  :/
>

Sure, I understand. I meant if it was possible or if that is too far
beyond its scope.

> >
> > I find Smatch interesting, especially when switching between projects
> > frequently. Does it support changing the code like clang-format? To
> > offload cognitive load to tools is usually good :).
>
> No.  Coccinelle does that really well though.
>

Understood.

Thanks!

> regards,
> dan carpenter
>

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

[Dan Carpenter]

On Fri, May 27, 2022 at 08:50:16AM +0200, Eugenio Perez Martin wrote:
> On Thu, May 26, 2022 at 9:07 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Thu, May 26, 2022 at 07:00:06PM +0200, Eugenio Perez Martin wrote:
> > > > It feels like returning any literal that isn't 1 or 0 should trigger a
> > > > warning...  I've written that and will check it out tonight.
> > > >
> > >
> > > I'm not sure this should be so strict, or "literal" does not include pointers?
> > >
> >
> > What I mean in exact terms, is that if you're returning a known value
> > and the function returns bool then the known value should be 0 or 1.
> > Don't "return 3;".  This new warning will complain if you return a known
> > pointer as in "return &a;".  It won't complain if you return an
> > unknown pointer "return p;".
> >
> 
> Ok, thanks for the clarification.
> 
> > > As an experiment, can Smatch be used to count how many times a
> > > returned pointer is converted to int / bool before returning vs not
> > > converted?
> >
> > I'm not super excited to write that code...  :/
> >
> 
> Sure, I understand. I meant if it was possible or if that is too far
> beyond its scope.

To be honest, I misread what you were asking.  GCC won't let you return
a pointer with an implied cast to int.  It has to be explicit.  So there
are zero of those.  It's not hard to look for pointers with an implied
cast to bool.

static void match_pointer(struct expression *ret_value)
{
        struct symbol *type;
        char *name;

        type = cur_func_return_type();
        if (!type || sval_type_max(type).value != 1)
                return;

        if (!is_pointer(ret_value))
                return;

        name = expr_to_str(ret_value);
        sm_msg("'%s' return pointer cast to bool", name);
        free_string(name);
}

regards,
dan carpenter

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

[Dawar, Gautam]

[AMD Official Use Only - General]

IMHO replacing "return ops->stop;" with "return ops->stop?true:false;" should be good enough.

On Fri, May 27, 2022 at 08:50:16AM +0200, Eugenio Perez Martin wrote:
> On Thu, May 26, 2022 at 9:07 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Thu, May 26, 2022 at 07:00:06PM +0200, Eugenio Perez Martin wrote:
> > > > It feels like returning any literal that isn't 1 or 0 should
> > > > trigger a warning...  I've written that and will check it out tonight.
> > > >
> > >
> > > I'm not sure this should be so strict, or "literal" does not include pointers?
> > >
> >
> > What I mean in exact terms, is that if you're returning a known
> > value and the function returns bool then the known value should be 0 or 1.
> > Don't "return 3;".  This new warning will complain if you return a
> > known pointer as in "return &a;".  It won't complain if you return
> > an unknown pointer "return p;".
> >
>
> Ok, thanks for the clarification.
>
> > > As an experiment, can Smatch be used to count how many times a
> > > returned pointer is converted to int / bool before returning vs
> > > not converted?
> >
> > I'm not super excited to write that code...  :/
> >
>
> Sure, I understand. I meant if it was possible or if that is too far
> beyond its scope.

To be honest, I misread what you were asking.  GCC won't let you return a pointer with an implied cast to int.  It has to be explicit.  So there are zero of those.  It's not hard to look for pointers with an implied cast to bool.

static void match_pointer(struct expression *ret_value) {
        struct symbol *type;
        char *name;

        type = cur_func_return_type();
        if (!type || sval_type_max(type).value != 1)
                return;

        if (!is_pointer(ret_value))
                return;

        name = expr_to_str(ret_value);
        sm_msg("'%s' return pointer cast to bool", name);
        free_string(name);
}

regards,
dan carpenter

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

[Dan Carpenter]

On Fri, May 27, 2022 at 10:36:55AM +0300, Dan Carpenter wrote:
> static void match_pointer(struct expression *ret_value)
> {
>         struct symbol *type;
>         char *name;
> 
>         type = cur_func_return_type();
>         if (!type || sval_type_max(type).value != 1)
>                 return;
> 
>         if (!is_pointer(ret_value))
>                 return;
> 
>         name = expr_to_str(ret_value);
>         sm_msg("'%s' return pointer cast to bool", name);
>         free_string(name);
> }

I found a bug in Smatch with this check.  With the Smatch bug fixed then
there is only only place which does a legitimate implied pointer to bool
cast.  A different function returns NULL on error instead of false.

drivers/gpu/drm/i915/display/intel_dmc.c:249 intel_dmc_has_payload() 'i915->dmc.dmc_info[0]->payload' return pointer cast to bool
drivers/net/wireless/rndis_wlan.c:1980 rndis_bss_info_update() '(0)' return pointer cast to bool
drivers/net/wireless/rndis_wlan.c:1989 rndis_bss_info_update() '(0)' return pointer cast to bool
drivers/net/wireless/rndis_wlan.c:1995 rndis_bss_info_update() '(0)' return pointer cast to bool

regards,
dan carpenter

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

[Dan Carpenter]

On Mon, May 30, 2022 at 05:27:25PM +0300, Dan Carpenter wrote:
> On Fri, May 27, 2022 at 10:36:55AM +0300, Dan Carpenter wrote:
> > static void match_pointer(struct expression *ret_value)
> > {
> >         struct symbol *type;
> >         char *name;
> > 
> >         type = cur_func_return_type();
> >         if (!type || sval_type_max(type).value != 1)
> >                 return;
> > 
> >         if (!is_pointer(ret_value))
> >                 return;
> > 
> >         name = expr_to_str(ret_value);
> >         sm_msg("'%s' return pointer cast to bool", name);
> >         free_string(name);
> > }
> 
> I found a bug in Smatch with this check.  With the Smatch bug fixed then
> there is only only place which does a legitimate implied pointer to bool
> cast.  A different function returns NULL on error instead of false.
> 
> drivers/gpu/drm/i915/display/intel_dmc.c:249 intel_dmc_has_payload() 'i915->dmc.dmc_info[0]->payload' return pointer cast to bool
> drivers/net/wireless/rndis_wlan.c:1980 rndis_bss_info_update() '(0)' return pointer cast to bool
> drivers/net/wireless/rndis_wlan.c:1989 rndis_bss_info_update() '(0)' return pointer cast to bool
> drivers/net/wireless/rndis_wlan.c:1995 rndis_bss_info_update() '(0)' return pointer cast to bool

Hm...  I found another.  I'm not sure why it wasn't showing up before.

lib/assoc_array.c:941 assoc_array_insert_mid_shortcut() 'edit' return pointer cast to bool

That's weird.  I will re-run the test.

regards,
dan carpenter