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

[Eugenio Perez Martin <eperezma@redhat.com>]

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?

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?

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?