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
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 <[email protected]>
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 21 +++++++++++++++++++++
drivers/vdpa/vdpa_sim/vdpa_sim.h | 1 +
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 +++
drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 +++
4 files changed, 28 insertions(+)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 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
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 <[email protected]>
---
drivers/vhost/vdpa.c | 16 +++++++++++++++-
include/uapi/linux/vhost_types.h | 2 ++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 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
[AMD Official Use Only - General]
-----Original Message-----
From: Eugenio Pérez <[email protected]>
Sent: Wednesday, May 25, 2022 4:29 PM
To: Michael S. Tsirkin <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; Jason Wang <[email protected]>
Cc: Zhu Lingshan <[email protected]>; [email protected]; Stefano Garzarella <[email protected]>; [email protected]; Eli Cohen <[email protected]>; Dan Carpenter <[email protected]>; Parav Pandit <[email protected]>; Wu Zongyong <[email protected]>; [email protected]; Christophe JAILLET <[email protected]>; Xie Yongji <[email protected]>; Dawar, Gautam <[email protected]>; [email protected]; [email protected]; [email protected]; Longpeng <[email protected]>; [email protected]; Kamde, Tanuj <[email protected]>; Si-Wei Liu <[email protected]>; [email protected]; [email protected]; Zhang Min <[email protected]>; [email protected]
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 <[email protected]>
---
drivers/vhost/vdpa.c | 16 +++++++++++++++-
include/uapi/linux/vhost_types.h | 2 ++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 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
This operation is optional: It it's not implemented, backend feature bit
will not be exposed.
Signed-off-by: Eugenio Pérez <[email protected]>
---
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
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);
}
On Thu, May 26, 2022 at 11:07 AM Stefano Garzarella <[email protected]> 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 <[email protected]> wrote:
> >>
> >> [AMD Official Use Only - General]
> >>
> >> -----Original Message-----
> >> From: Eugenio Pérez <[email protected]>
> >> Sent: Wednesday, May 25, 2022 4:29 PM
> >> To: Michael S. Tsirkin <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; Jason Wang <[email protected]>
> >> Cc: Zhu Lingshan <[email protected]>; [email protected]; Stefano Garzarella <[email protected]>; [email protected]; Eli Cohen <[email protected]>; Dan Carpenter <[email protected]>; Parav Pandit <[email protected]>; Wu Zongyong <[email protected]>; [email protected]; Christophe JAILLET <[email protected]>; Xie Yongji <[email protected]>; Dawar, Gautam <[email protected]>; [email protected]; [email protected]; [email protected]; Longpeng <[email protected]>; [email protected]; Kamde, Tanuj <[email protected]>; Si-Wei Liu <[email protected]>; [email protected]; [email protected]; Zhang Min <[email protected]>; [email protected]
> >> 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 <[email protected]>
> >> ---
> >> drivers/vhost/vdpa.c | 16 +++++++++++++++-
> >> include/uapi/linux/vhost_types.h | 2 ++
> >> 2 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 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!
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 <[email protected]> wrote:
>>
>> [AMD Official Use Only - General]
>>
>> -----Original Message-----
>> From: Eugenio P?rez <[email protected]>
>> Sent: Wednesday, May 25, 2022 4:29 PM
>> To: Michael S. Tsirkin <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; Jason Wang <[email protected]>
>> Cc: Zhu Lingshan <[email protected]>; [email protected]; Stefano Garzarella <[email protected]>; [email protected]; Eli Cohen <[email protected]>; Dan Carpenter <[email protected]>; Parav Pandit <[email protected]>; Wu Zongyong <[email protected]>; [email protected]; Christophe JAILLET <[email protected]>; Xie Yongji <[email protected]>; Dawar, Gautam <[email protected]>; [email protected]; [email protected]; [email protected]; Longpeng <[email protected]>; [email protected]; Kamde, Tanuj <[email protected]>; Si-Wei Liu <[email protected]>; [email protected]; [email protected]; Zhang Min <[email protected]>; [email protected]
>> 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 <[email protected]>
>> ---
>> drivers/vhost/vdpa.c | 16 +++++++++++++++-
>> include/uapi/linux/vhost_types.h | 2 ++
>> 2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 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
On Wed, May 25, 2022 at 1:23 PM Dawar, Gautam <[email protected]> wrote:
>
> [AMD Official Use Only - General]
>
> -----Original Message-----
> From: Eugenio Pérez <[email protected]>
> Sent: Wednesday, May 25, 2022 4:29 PM
> To: Michael S. Tsirkin <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; Jason Wang <[email protected]>
> Cc: Zhu Lingshan <[email protected]>; [email protected]; Stefano Garzarella <[email protected]>; [email protected]; Eli Cohen <[email protected]>; Dan Carpenter <[email protected]>; Parav Pandit <[email protected]>; Wu Zongyong <[email protected]>; [email protected]; Christophe JAILLET <[email protected]>; Xie Yongji <[email protected]>; Dawar, Gautam <[email protected]>; [email protected]; [email protected]; [email protected]; Longpeng <[email protected]>; [email protected]; Kamde, Tanuj <[email protected]>; Si-Wei Liu <[email protected]>; [email protected]; [email protected]; Zhang Min <[email protected]>; [email protected]
> 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 <[email protected]>
> ---
> drivers/vhost/vdpa.c | 16 +++++++++++++++-
> include/uapi/linux/vhost_types.h | 2 ++
> 2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 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
>
On Thu, May 26, 2022 at 9:07 PM Dan Carpenter <[email protected]> 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
>
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 <[email protected]> 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
On Thu, May 26, 2022 at 3:21 PM Dan Carpenter <[email protected]> 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);
> }
>
[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 <[email protected]> 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
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
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
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