| Message ID | 20220525105922.2413991-3-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Implement vdpasim stop operation | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
[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
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 >
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
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!
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 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); > } >
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 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 >
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
[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
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
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
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
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(-)