Re: [PATCH v2 3/5] vduse: add VDUSE_GET_FEATURES ioctl

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Mar 03, 2026 at 03:20:22PM +0800, Jason Wang wrote:
> On Fri, Feb 13, 2026 at 3:39 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Fri, Feb 13, 2026 at 2:25 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Feb 12, 2026 at 4:23 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Thu, Feb 12, 2026 at 9:07 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Tue, Feb 10, 2026 at 4:26 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > Add an ioctl to allow VDUSE instances to query the available features
> > > > > > supported by the kernel module.
> > > > > >
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > ---
> > > > > > A simple u64 bitmap is used for feature flags. While a flexible array
> > > > > > could support indefinite expansion, 64 bits is sufficient for the
> > > > > > foreseeable future and simplifies the implementation.
> > > > > >
> > > > > > Also, dev_dbg is used for logging rather than dev_err as these can be
> > > > > > triggered from userspace.
> > > > > > ---
> > > > > > v2:
> > > > > > * return -EINVAL if ioctl called with version < 2, so userland visible
> > > > > >   reply is kept (Jason).
> > > > > > ---
> > > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 28 ++++++++++++++++++++++++++++
> > > > > >  include/uapi/linux/vduse.h         |  7 ++++++-
> > > > > >  2 files changed, 34 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > index d1da7c15d98b..7458cbaed4c7 100644
> > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > @@ -52,6 +52,9 @@
> > > > > >
> > > > > >  #define IRQ_UNBOUND -1
> > > > > >
> > > > > > +/* Supported VDUSE features */
> > > > > > +static const uint64_t vduse_features;
> > > > > > +
> > > > > >  /*
> > > > > >   * VDUSE instance have not asked the vduse API version, so assume 0.
> > > > > >   *
> > > > > > @@ -1947,6 +1950,19 @@ static bool vduse_validate_config(struct vduse_dev_config *config,
> > > > > >                          sizeof(config->reserved)))
> > > > > >                 return false;
> > > > > >
> > > > > > +       if (api_version < VDUSE_API_VERSION_2) {
> > > > > > +               if (config->vduse_features) {
> > > > > > +                       dev_dbg(vduse_ctrl_dev,
> > > > > > +                               "config->vduse_features with version %llu",
> > > > > > +                               api_version);
> > > > > > +                       return false;
> > > > > > +               }
> > > > > > +       } else {
> > > > > > +               if (config->vduse_features & ~vduse_features)
> > > > > > +                       return false;
> > > > > > +       }
> > > > > > +
> > > > > > +
> > > > > >         if (api_version < VDUSE_API_VERSION_1 &&
> > > > > >             (config->ngroups || config->nas))
> > > > > >                 return false;
> > > > > > @@ -2207,6 +2223,18 @@ static long vduse_ioctl(struct file *file, unsigned int cmd,
> > > > > >                 ret = vduse_destroy_dev(name);
> > > > > >                 break;
> > > > > >         }
> > > > > > +       case VDUSE_GET_FEATURES:
> > > > > > +               if (control->api_version < VDUSE_API_VERSION_2) {
> > > > > > +                       dev_dbg(vduse_ctrl_dev,
> > > > > > +                               "VDUSE_GET_FEATURES ioctl with version %llu",
> > > > > > +                               control->api_version);
> > > > > > +                       ret = -EINVAL;
> > > > > > +                       break;
> > > > > > +               }
> > > > > > +
> > > > > > +               ret = put_user(vduse_features, (u64 __user *)argp);
> > > > > > +               break;
> > > > > > +
> > > > > >         default:
> > > > > >                 ret = -EINVAL;
> > > > > >                 break;
> > > > > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > > > > index 27832d46084c..8898d9daa777 100644
> > > > > > --- a/include/uapi/linux/vduse.h
> > > > > > +++ b/include/uapi/linux/vduse.h
> > > > > > @@ -37,6 +37,7 @@
> > > > > >   * @vq_align: the allocation alignment of virtqueue's metadata
> > > > > >   * @ngroups: number of vq groups that VDUSE device declares
> > > > > >   * @nas: number of address spaces that VDUSE device declares
> > > > > > + * @vduse_features: VDUSE features
> > > > > >   * @reserved: for future use, needs to be initialized to zero
> > > > > >   * @config_size: the size of the configuration space
> > > > > >   * @config: the buffer of the configuration space
> > > > > > @@ -53,7 +54,8 @@ struct vduse_dev_config {
> > > > > >         __u32 vq_align;
> > > > > >         __u32 ngroups; /* if VDUSE_API_VERSION >= 1 */
> > > > > >         __u32 nas; /* if VDUSE_API_VERSION >= 1 */
> > > > > > -       __u32 reserved[11];
> > > > > > +       __u64 vduse_features;
> > > > >
> > > > > Nit: Should we document " /* if VDUSE_API_VERSION >= 2 */
> > > > >
> > > > > But I have an open question, is this better to just introduce a
> > > > > VDUSE_SET_FEATURES then we can do negotiation to avoid bumping API
> > > > > versions?
> > > > >
> > > >
> > > > How do you know if you can call VDUSE_GET_FEATURES and
> > > > VDUSE_SET_FEATURES then? vduse_ioctl does not even return -ENOIOCMD
> > > > but -EINVAL if you call the equivalent of VDUSE_GET_FEATURES on
> > > > current Linux master. Or should the VDUSE instance assume that if the
> > > > ioctl returns -EINVAL the kernel does not support them?
> > >
> > > If we go that way, yes.
> > >
> >
> > I'm ok with doing the change. Kind of, I still think that increasing
> > the API version is way cleaner, so no ioctl returns any error in
> > regular initialization. But python's style "ask forgiveness not
> > permission" is also ok for me.
> >
> > But I'm really missing the motivation of it. What's the problem with
> > the increase of the version that you are trying to solve here?
> 
> For example, if a feature is simply added via a new ioctl. I think we
> don't need to bump the version. An example is the umem reg/dereg.
> VDUSE starts from this choice so I think we should stick to that.
> 
> I think a better time to bump the version are
> 
> 1) an ioctl is expected to behave differently
> 2) new ioctls are mutually exclusive with one(s) of the existing ioctls
> 3) new initialization protocol
> 
> etc.
> 
> >
> > Can we make a rule to know when to increase the version, and when it
> > is not needed and it is ok for ioctls to just return -EINVAL or
> > -ENOIOCTLCMD, so future series can hit the right behavior from earlier
> > versions?
> 
> See above.
> 
> >
> > From your comment I extract that we don't need to increase the API
> > version number if we just add ioctls commands to the device, like
> > probing if an ioctl exists. Also, we don't need to increase the
> > version if we can tell if a member in vduse_dev_config is known to
> > exist because a previous ioctl (or feature flag) succeeded.
> >
> > Am I missing something?
> 
> I might be wrong, just some thoughts. Please correct me if I was wrong.
> 
> Thanks
> 
> >


Not bumping version is nice as people can cherry pick features.

OTOH if people are cherry picking features then userspace has
to support all kind of combinations of features.


So I think there can not be a simple rule, it boils down to this:
if userspace can handle all failures easily - just add an ioctl.

If userspace can not and would merely assert on success -
update the version.


-- 
MST